Repository navigation
Add OptogenSIMLight: Monte Carlo–based light model - #74
Conversation
…age; export from cleo.light
Zero transmittance only beyond the data's negative-z range rather than blanket-zeroing everything behind the source, preserving near-source backscatter. Update test to assert small positive backscatter within range and hard zero beyond it.
Previous sweep used stale tissue properties (fixed at startup wavelength) for all 78 runs. Fixed OptogenSIM GO loop to recompute tissue per wavelength. Penetration now traces hemoglobin absorption: shallowest at 410nm (0.33mm, Soret peak), deepest in red (~1.6mm), Q-band dip at 560nm.
Both fibers (590nm amber, 470nm blue) now use OptogenSIMLight. Amber previously used OpticFiber blue coefficients scaled 0.8x as a 590nm stand-in; now uses real per-wavelength MC data. Removed redundant additive demo section. Narrative claims verified against regenerated figures.
|
Found 2 changed notebooks. Review the changes at https://app.gitnotebooks.com/siplab-gt/cleo/pull/74 |
kjohnsen
left a comment
kjohnsen
left a comment
There was a problem hiding this comment.
Marvelous! Some refinements to be made, but an excellent start.
…sed_gaussian_beam constructor
…monotonicity tests
Factory docstring now references OptogenSIM for wavelength/beam_radius rather than restating them, keeping parameter descriptions in one place. Retains data_path handling since the factory's role is file loading.
|
Pushed a round of updates addressing the recent review comments — the multi_opto light tuning, removing the unused enphr3 opsin, the factory docstring cleanup, and the wavelength refactor moving it to the LightModel. Re-requesting review so you can take a look at the updated changes whenever you get a chance. A few threads are still open with questions for you where I wasn't sure of the direction. Thanks! |
kjohnsen
left a comment
kjohnsen
left a comment
There was a problem hiding this comment.
Looking very nice! Just a few more refinements to make.
optogensim.py: - Remove unnecessary slots=False (cached_property works slotted) - Remove data_path from defocused_gaussian_beam - Expand factory + data docstrings (structure, units, ranges; repo link to follow) - Extend viz to show backscatter behind the source - Default n_points_per_source to 16000 to reduce marker size Also: declare xarray+plotly in pyproject; update light_viz Light(wavelength=) call sites to the new light-model wavelength API.
Generalized test_OptogenSIM_T_increases_with_beam_radius into test_T_increases_with_beam_width, parametrized across fiber (R0), GaussianEllipsoid (sigma_lateral), and OptogenSIM (beam_radius), each with a model-appropriate target point. Addresses review comment.
Replace direct xarray loading with the OptogenSIM model (simpler, acts as an integration test). Set heatmap axes to equal scale (scaleanchor) so 1mm is the same vertically and horizontally. Note: marimo run still errors, but the cause is brian2 installing a SIGINT handler at import (fails in marimo run's worker thread), not the file-loading. marimo edit works.
|
Re: the registry warnings (multi_opto thread): Investigated this — you're right about the cause. When a second light is injected, init_register_light removes the old light_source_ng and light_prop_syns with _remove_brian_object (which calls network.remove()) and recreates them combined. The warning fires because network.remove() takes them out of the Network, but brian2 still tracks them from creation, so they warn when garbage-collected. A few ways to fix it: dispose of the removed objects so brian2 doesn't flag them, suppress the warning at removal, or restructure to avoid the throwaway objects. Do you have a preference? |
Deleting the objects would be cleanest I think |
When a second light is injected, init_register_light removes the old light_source_ng and light_prop_syns and rebuilds combined ones. brian2's BrianObject.__del__ warned these discarded objects were 'never included in a network' (fires when _network is None at GC). Set _network to network.id in _remove_brian_object after removal so the intentional discards don't warn. multi_opto now runs clean; full suite passes (269).
|
Worked through the rest of the review — registry warnings fixed (multi_opto runs clean now), and the OptogenSIM fork is published and linked in the docstring. Thank You!! |
There was a problem hiding this comment.
Looking wonderful. Just a teeny tiny bit more on the light profile notebook, to better function as an integration test
FYI I pushed some trivial changes I made as I tested and reviewed locally
Oh, and looks like some merge conflicts with master now...will have to merge master here and resolve for this branch to be ready to merge
…ments # Conflicts: # docs/tutorials/multi_opto.ipynb # docs/tutorials/optogenetics.ipynb # pyproject.toml # tests/light/test_light.py
- Add comment explaining the _network warning workaround in registry - Rework light_profile_explorer to call model.transmittance() with a coordinate grid instead of _rz_slice; plot raw transmittance (no scaling) - Recalibrate multi_opto amplitude (0.2 -> 8) for the merged aggregation fix
|
Addressed the latest comments. Reworked the profile explorer to use model.transmittance(), dropped the scaling, and added the registry comment. Also merged master and recalibrated the multi_opto amplitude for the aggregation fix. Tests pass locally. On the red CI check: the actual "Run tests" step passes. What fails is the "Post Set up Python" cleanup step, with Error: Cache folder path is retrieved for pip but doesn't exist on disk: /home/runner/.cache/pip. That looks an issue from the GitHub Actions Node 20 to 24 migration rather than anything in this PR, so it may need a workflow update (bumping setup-python or disabling pip cache) on the repo side. Ready for another look!! |
kjohnsen
left a comment
kjohnsen
left a comment
There was a problem hiding this comment.
Awesome. Down to 2 small things:
- we need to issue a warning for going outside the data range (and confirm behavior. I see code to zero out along z axis where outside data, but not R. Does that mean we simply inherit whatever xarray does?)
- Could you take a stab at fixing the CI problems? Try upgrading the whole uv/setup-python flow. Look at the most recent documentation for the uv action to see the recommended way to do it
transmittance now warns if any r/z target coordinate is outside the simulated data range, and documents the behavior (r/z clipped to the data edge; z beyond the near edge zeroed). Adds a unit test checking the warning fires and out-of-range output stays finite in [0, 1].
|
Added the data-range warning: transmittance now warns when target r/z fall outside the OptogenSIM data range, plus a unit test checking the warning fires and out-of-range output stays finite/clipped. (Documented the behavior: r/z clip to the data edge, z beyond the near edge zeroes out.) On the CI fix: the failure is actions/setup-python's pip-cache step breaking on the Node 24 runners (the project uses uv, not pip). Per the setup-uv docs, the fix is to drop setup-python and have setup-uv install Python via python-version. I have it ready but my token lacks workflow scope to push to .github/workflows/. Could you apply this, or let me know if I should get access? yaml |
Out-of-range targets (r beyond far edge, z outside grid) now get transmittance 0 instead of the clipped edge value, and the warning says so. Targets nearer the axis than the first radial bin keep the near-axis value. far_behind test expects the warning.
actions/setup-python's pip-cache post step fails on the Node 24 runners (pip cache dir missing; project uses uv, not pip). Bump checkout v3->v5 and setup-uv v5->v10, pass python-version through setup-uv instead. Also correct a stale peak-normalization comment in the profile explorer.
kjohnsen
left a comment
kjohnsen
left a comment
There was a problem hiding this comment.
Both items addressed: out-of-range transmittance now warns and zeros (with tests incl. peak-normalization), CI fixed and green across the matrix.
2 participants
Summary
Adds OptogenSIMLight, a light model built from OptogenSIM Monte Carlo simulations of light propagation through gray matter, as a simulation alternative to the analytical OpticFiber (Foutz 2012). Built from a 78-run sweep (13 wavelengths (380–740 nm) × 6 beam radii (10–800 µm)) assembled into a 4D dataset.
What's included
Design decisions
Dataset generation & validation
Tissue optical properties follow Jacques (2013, Phys. Med. Biol. 58:R37), which makeTissueList_OGS implements. The dataset was regenerated after finding the original sweep held tissue properties fixed across all wavelengths (the sweep loop didn't recompute the tissue list per wavelength, so wavelength-dependence wasn't captured). After fixing, penetration correctly tracks the tissue absorption spectrum: shallowest near the 410 nm hemoglobin Soret peak (~0.33 mm), a Q-band dip at 560 nm, deepest in the red (~1.5 mm) which is consistent with the Jacques model, in agreement with OpticFiber/Aravanis, and within the literature range for red light in gray matter.
Testing
Full light test suite passes (132 tests).
Dataset validated: penetration traces the absorption spectrum across all 13 wavelengths, beam width scales with beam size, wavelength interpolation stays in bounds.