Raster Scanning Code by GlennZZY · Pull Request #55 · siplab-gt/cleo

GlennZZY · 2024-11-15T03:01:45Z

Uploaded the code for raster scanning.

Since there's only one registry for simulation, the flag might not work at this moment

kjohnsen · 2024-11-20T16:55:22Z

cleo/imaging/scope.py

        factory=list, repr=False, init=False
    )
    """relative expression levels of neurons selected from each injection"""
+    Registry: DeviceInteractionRegistry = None


Are you sure we need to store the registry when we can just do registry_for_sim(self.sim)?

kjohnsen · 2024-11-20T20:20:09Z

cleo/imaging/scope.py

        self.focus_coords_per_injct.append(focus_coords)
        self.rho_rel_per_injct.append(rho_rel)
+        self.Registry = registry_for_sim(self.sim)
+        self.Registry.update_fov(self.img_width)


would this need to be updated on every injection? Also, it can't be right that there's a single update_fov for the whole registry, since this should vary for each light device

the Scope should optionally have an imaging_light object, and the img_width and scan_freq would be updated specific to that light

kjohnsen · 2024-11-20T21:44:49Z

cleo/registry.py


+    raster_fov: int = field(default=500 * 1e-6 * meter, kw_only=True)
+
+    raster_enable: int = field(default=0, kw_only=True)


again, these would be at the level of individual lights, not the whole registry

kjohnsen · 2024-11-20T21:49:26Z

cleo/light/light.py

    wavelength: Quantity = field(default=473 * nmeter, kw_only=True)
    """light wavelength with unit (usually nmeter)"""

+    scan_freq : int = field(default=30, kw_only=True)


I'd create a subclass RasterScanningLight to hide the scan_freq details from the typical Light object

kjohnsen · 2024-11-20T21:54:20Z

cleo/registry.py

        i_source = self.subgroup_idx_for_light[light]
        light_prop_syn.epsilon[i_source, :] = epsilon
        light_prop_syn.T[i_source, :] = light.transmittance(coords_from_ng(ng)).ravel()
+        light_prop_syn.scan_period = second / light.scan_freq


I'd put these in a separate function that you call both here (on injection) and when you change the params on the scope/imaging light (setter functions). like update_raster_scan_params(scope) or something like that

like you've already done with update_fov but can include these other variables too

kjohnsen

Looks pretty good! Besides the comments below, as I was about to test it (with the script at the bottom that plots all the action spectra), it occurred to me it's getting to be too much to check just by looking at the numbers. I think we need a new tutorial notebook that goes over all the opsins that will serve two puposes: (1) quickly introduce the user to all the options and (2) let us visually inspect that all the opsins are behaving as expected. Can you create a docs/tutorials/opsin_selection.ipynb notebook where we plot 4 things for every opsin?

The 1P action spectrum,
the 2P action spectrum,
the impulse response (the photocurrent after a quick (e.g., 5 ms), strong pulse),
and the pulse response (what the photocurrent looks like with prolonged stimulation (should see peak, then back down to plateau)

Since there are now too many opsins to visualize all at once, can you break this into groups? For example, plot all the ChR2 variants together, all the Chrimson variants together, all the inhibitory pumps together, all the ChRmine variants together?

kjohnsen · 2025-04-02T20:59:19Z

.DS_Store

can we remove these? we should add **/.DS_Store to .gitignore

cleo/opto/opsin_library.py

kjohnsen · 2025-04-02T22:04:27Z

docs/tutorials/optogenetics.ipynb

 "metadata": {
  "kernelspec": {
-   "display_name": "python3",
+   "display_name": ".venv",


no need to commit this notebook if you didn't change it. Also, if you did change the notebook, use nbdev clean to get rid of stuff like this before committing

cleo/opto/opsin_library.py

kjohnsen · 2025-04-03T13:26:42Z

cleo/opto/opsin_library.py

+            (490, 0.92),
+            (498, 1),
+            (516, 0.84),
+            (507, 0.90),


here's another place the spectrum is out of order. For legibility and reproducibility, it should be in order, but I remembered I actually should have already accounted for this in light_dependence.py, line 125...don't know why I'm getting an error. Could you add a unit test for this? I guess it would be a new file tests/light/test_light_dependence.py

kjohnsen

The overall pattern of creating a light from the scope looks good! Just time to put it all together, make some tweaks, and most importantly, get it working in an end-to-end example and in unit tests

kjohnsen · 2025-07-25T17:50:56Z

cleo/imaging/scope.py

        self.rho_rel_per_injct.append(rho_rel)
-        self.Registry = registry_for_sim(self.sim)
-        self.Registry.update_fov(self.img_width)
+        registry_for_sim(self.sim).update_fov(self.img_width)


I changed this in 0.18 so the registry is stored on sim. so self.sim.registry from here on

kjohnsen · 2025-07-25T17:52:52Z

cleo/imaging/scope.py

+    def create_imaging_light(self):
+        self.imaging_light = Light(
+            name=f"{self.name}_imlight",
+            light_model=GaussianEllipsoid(radius=self.img_width/2),


I would give the user the option to pass in things like wavelength. And radius would be the radius of a neuron (soma_radius). And don't inject for the user; require them to inject manually (since you inject on a per-neuron group basis)

kjohnsen · 2025-07-25T17:54:06Z

cleo/imaging/scope.py

+        registry_for_sim(self.sim).set_scan_on(self.imaging_light, flag)
+
+    # change frame rate
+    def set_scan_freq(self, hz: float):


I would use Python's built-in setter/getter syntax to make this nicer to use and more Pythonic: https://realpython.com/python-getter-setter/

kjohnsen · 2025-07-25T18:05:54Z

cleo/light/light.py

    def init_for_simulator(self, sim: CLSimulator) -> None:
        registry = registry_for_sim(sim)
        registry.init_register_light(self)
+        """Test Code for setter method"""


I assume you'd move this to test_scope.py?

kjohnsen · 2025-07-25T18:08:19Z

cleo/registry.py

+            setattr(src, k, v)
+
+    def set_scan_on(self, light: "Light", enable: bool = True):
+        self._apply_to_source(light, is_scanning=int(bool(enable)))


nice idea with the _apply_to_source to reuse the logic of getting the imaging light subgroup. I'd use a more obvious name like _apply_to_img_light though

kjohnsen · 2025-07-25T18:09:21Z

cleo/registry.py

        # fmt: on
+        src = self.source_for_light(light)
+        src.scan_period = (1 / light.scan_freq) * second
+        spot_radius = 1e-6 * meter


these are already defined by the scope and shouldn't be redefined here

kjohnsen · 2025-07-25T18:13:01Z

cleo/registry.py

-        if self.raster_fov != new_fov:
-            self.raster_fov = new_fov
-            warnings.warn("Warning: cleo currently supports use of single scanning fov in simulation, the latest update parameter will be used", UserWarning)
+    def set_fov(self, light: "Light", fov):


I think would should stick with img_width instead of fov for consistency with Scope and also because from what I understand, field of view is a technical term measured as an angle

Create Branch

34465d7

kjohnsen reviewed Nov 20, 2024

View reviewed changes

kjohnsen assigned kjohnsen and GlennZZY and unassigned kjohnsen Nov 20, 2024

randismaael added 2 commits February 14, 2025 16:28

Merge branch 'master' into raster-scanning

25f6828

Opsin Library Update

d9d00ab

kjohnsen requested changes Apr 3, 2025

View reviewed changes

Updated Raster Scanning code

edc09d5

kjohnsen requested changes Jul 25, 2025

View reviewed changes


		raster_fov: int = field(default=500 * 1e-6 * meter, kw_only=True)

		raster_enable: int = field(default=0, kw_only=True)

Conversation

GlennZZY commented Nov 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjohnsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjohnsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants