Conversation
kjohnsen
left a comment
kjohnsen
left a comment
There was a problem hiding this comment.
Looking pretty good. Main things:
- the features and interface can and should be reused more with S2F (i.e., in a Sensor base class)
- the tests can be unified and a little stronger
- the functions for different sensors are getting a bit out of hand....what if we kept a csv file with all the available sensors and parameters? Where a param is missing, we can leave it blank, and throw an error when trying to load a model that needs it.
- This would be more elegant for parameters shared between S2F and NAOMi (
sigma_noise,dFF_1AP)
- This would be more elegant for parameters shared between S2F and NAOMi (
There was a problem hiding this comment.
Is there a reason you removed the Sensor base class? It would be even more important now that this functionality is shared between multiple child classes.
There was a problem hiding this comment.
Oh...because Sensor was a SynapseDevice...ok, well hopefully we can still do this with NAOMiGECI inheriting from SynapseDevice and Sensor and S2FGECI just the latter
| class S2FGECI(SynapseDevice): | ||
| """Base class for S2F GECI models""" | ||
|
|
||
| rise: Quantity = field(kw_only=True) |
There was a problem hiding this comment.
We will need docstrings for all these things. Don't worry about it till everything else is done though; best not to waste time documenting things that are in flux
| F0: float = field(kw_only=True, default=0.0) | ||
| sigma_noise: float = field(kw_only=True, default=0.0) | ||
| threshold: float = field(kw_only=True, default=1e-3) | ||
| spike_monitors: dict[NeuronGroup, SpikeMonitor] = field(factory=dict, init=False) |
There was a problem hiding this comment.
We'd probably want this to be private.
Also, if it's a dict with NG keys, how do you handle multiple injections into the same neuron group? Or do we take a shortcut assuming you inject all at the same time with scope.inject_sensor_for_targets()? I don't remember
There was a problem hiding this comment.
Can we combine tests for NAOMi and S2F? Ideally, the existing NAOMi test_geci() test should work for both. And your S2F tests should too. I would keep a single test_sensor.py file and programmatically plug in both.
| assert isinstance(model_light, S2FLightDependentGECI) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
rather than parametrizing an s2f-specific test with s2f-specific params, you could refactor this as a general GECI test parameterized by different sensors
| sigma_noise=0.01, | ||
| ) | ||
| model.connect_to_neuron_group(ng) | ||
| assert "test_ng" in model.spike_monitors |
There was a problem hiding this comment.
We could use a test of tricky multi-inject scenarios
There was a problem hiding this comment.
We should remove these meaningless notebook changes from the PR
There was a problem hiding this comment.
Why don't S2F sensors use focus_depth like NAOMi? That should be a feature general to all sensors (including voltage sensors which we haven't thought about implementing yet).
There was a problem hiding this comment.
It would be nice to see the spike pattern to compare to the resulting Ca++ traces
| packages = [{ include = "cleo" }] | ||
|
|
||
| [tool.poetry.dependencies] | ||
| python = ">=3.9,<3.13" |
There was a problem hiding this comment.
I had a limit on the minor version since sometimes new Python versions break things. But we can try it. Poetry has this ^ operator that means anything the same or higher within the major version
There was a problem hiding this comment.
| python = ">=3.9,<3.13" | |
| python = "^3.9" |
2 participants
Add S2F model for calcium imaging.