-
Notifications
You must be signed in to change notification settings - Fork 0
add filters for circuit in simulation campaign #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9773916
to
17a6c0b
Compare
@@ -18,6 +19,7 @@ | |||
class SimulationFilterBase(NameFilterMixin, IdFilterMixin, CustomFilter): | |||
entity_id: uuid.UUID | None = None | |||
entity_id__in: list[uuid.UUID] | None = None | |||
circuit: Annotated[NestedCircuitFilter | None, NestedCircuitFilterDep] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I am ok with this workaround, I am afraid that someone will decide to set a different entity in entity_id breaking this endpoint. On the other hand we control what is added in Simulation's entity_id, so we just need to be careful. However, people forget..
@GianlucaFicarelli wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can modify the filter when we'll allow different types of entities, but do we already know which ones to expect? Not sure how easy is to use a sort of polymorphism here, or if we need different filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK, probably other simulations (single neuron, synaptome) will be using the same logic in the future
17a6c0b
to
6a8c1e2
Compare
Can you please add.a test that covers this use case? Just to make sure we don't break it unintentionally in the future. |
forget to push them, test added |
@GianlucaFicarelli is this okey to be merged ? |
dc882b2
to
a386697
Compare
No description provided.