[TRCL-658][feat] SPARC: support for LabCube after spectrograph#3339
Conversation
…fter spectrograph
Allow to force the protection from a separate component. This can be useful in case another component can also cause some light in.
If a (optical fiber coupled to a) time-correlator is connected to the spectrograph output, it should also have the MD_OUT_WL metadata stored, similarly to the monochromator.
If a (optical fiber coupled to a) time-correlator is connected to the spectrograph output, then the position of the spectograph input slit matters. Note: this actuator position will only be changed if it "affects" (directly or indirectly) the time-correlator. So, on standard SPARCs where the time-correlator is coupled before the spectrograph, this change has no effect.
Many functions have "main_data = self._main_data_model". For these function, make sure to always use directly "main_data", instead of refering to "self._main_data_model". That mostly makes the code more readable... and should be a tiny bit faster.
…photo-detector count When adjusting the alignment of the fiber when a labcube is coupled after the spectrograph, seeing the detector count over time can be useful. Otherwise, the only view possible is the immediate count (via odemis live). As this alignment is normally done only once per installation, and there is only one installation, it's currently just a plugin. If it needs to be done more frequently, we could add a new alignment mode to the alignment tab.
pieleric
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for time-correlator streams when the LabCube is positioned after a spectrograph in a SPARC microscope configuration. The changes enable the time-correlator and photo-detector components to properly handle spectrograph axes (wavelength, grating, slits) in addition to the LabCube axes (density, filter).
Changes:
- Extended metadata updater to support time-correlator and photo-detector roles with spectrograph coupling
- Enhanced time-correlator stream configuration to optionally include spectrograph axes
- Added new photo-detector live display plugin for alignment purposes
- Improved LED protection mechanism in TMCM motor driver with manual override capability
- Refactored stream_bar code for consistency (self._main_data_model → main_data)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/odemisd/mdupdater.py | Extended role checking to include time-correlator and photo-detector for spectrograph metadata updates |
| src/odemis/gui/cont/stream_bar.py | Enhanced _getAffectingSpectrograph with default parameter; added spectrograph support to addTimeCorrelator; code consistency refactoring; fixed axes ordering in addMonochromator |
| src/odemis/gui/conf/data.py | Added spectrograph axis configurations (wavelength, grating, slits, filter) for ScannedTemporalSettingsStream |
| src/odemis/driver/tmcm.py | Added protection VA for manual LED protection control; refactored LED protection logic into _switch_led_prot method; improved _expected_do_pos tracking |
| src/odemis/acq/path.py | Added slit-in-big configuration for time-correlator mode |
| plugins/photo_det_live.py | New plugin providing live stream creation for photo-detectors with spectrograph and LabCube axes |
| plugins/spectrum_raw.py | Updated _getAffectingSpectrograph call to use new signature with default parameter |
| plugins/spectrum_arbscor.py | Updated _getAffectingSpectrograph call to use new signature with default parameter |
| plugins/la_spec.py | Updated _getAffectingSpectrograph call to use new signature with default parameter |
| plugins/blanker_spectrum.py | Updated _getAffectingSpectrograph call to use new signature with default parameter |
| @@ -0,0 +1,99 @@ | |||
| # -*- coding: utf-8 -*- | |||
| """ | |||
| Created on 5 Dev 2025 | |||
There was a problem hiding this comment.
Typo in date: "Dev" should be "Dec" (December).
| Created on 5 Dev 2025 | |
| Created on 5 Dec 2025 |
| axes.update({ | ||
| "wavelength": ("wavelength", spg), | ||
| "grating": ("grating", spg), | ||
| "iris-in": ("iris-in", spg), | ||
| "slit-in": ("slit-in", spg), | ||
| "slit-monochromator": ("slit-monochromator", spg), | ||
| }) |
There was a problem hiding this comment.
The code unconditionally adds spectrograph axes to the axes dictionary without checking if spg is None. When no spectrograph affects the detector and main_data.spectrograph is also None (which can happen in some microscope configurations), spg will be None, leading to axis map entries with None components. This pattern is inconsistent with addTimeCorrelator (line 2009 in stream_bar.py) which checks if spg is truthy before adding spectrograph axes. Consider wrapping the axes.update() call in an if spg: condition similar to how it's done in addTimeCorrelator.
| axes.update({ | |
| "wavelength": ("wavelength", spg), | |
| "grating": ("grating", spg), | |
| "iris-in": ("iris-in", spg), | |
| "slit-in": ("slit-in", spg), | |
| "slit-monochromator": ("slit-monochromator", spg), | |
| }) | |
| if spg: | |
| axes.update({ | |
| "wavelength": ("wavelength", spg), | |
| "grating": ("grating", spg), | |
| "iris-in": ("iris-in", spg), | |
| "slit-in": ("slit-in", spg), | |
| "slit-monochromator": ("slit-monochromator", spg), | |
| }) |
|
|
||
| @author: Éric Piel | ||
|
|
||
| Gives ability to acquire a spectrum data, while keeping the raw CCD image (ie, without vertical binning) |
There was a problem hiding this comment.
The description states "Gives ability to acquire a spectrum data, while keeping the raw CCD image (ie, without vertical binning)" but this plugin is for photo-detectors, not CCD cameras. Photo-detectors typically don't have binning options and don't produce images. The description appears to be copied from another plugin (possibly spectrum_raw.py) without being updated for this plugin's actual purpose.
| Gives ability to acquire a spectrum data, while keeping the raw CCD image (ie, without vertical binning) | |
| Provides live display and control of photo-detector signals for SPARC systems using monochromator settings. |
| # always created when a time-correlator is available, which is the assumption here. | ||
|
|
||
| def add_photo_det_stream(self, name: str, detector: model.Detector): | ||
| """ Create a Monochromator stream, using a photo-detector and add to to all compatible viewports""" |
There was a problem hiding this comment.
Typo in docstring: "add to to all compatible viewports" should be "add to all compatible viewports" (remove duplicate "to").
| """ Create a Monochromator stream, using a photo-detector and add to to all compatible viewports""" | |
| """ Create a Monochromator stream, using a photo-detector and add to all compatible viewports""" |
| self._expected_do_pos[an] = v | ||
| with self._refswitch_lock: | ||
| self._expected_do_pos[an] = v # Update user-requested position | ||
| if channel in self._led_prot_do and (self.protection.value or self._leds_on): |
There was a problem hiding this comment.
The code accesses self.protection.value without checking if the attribute exists. While this should be safe because self.protection is only created when self._led_prot_do is not empty (line 463-467), and this code is inside a condition checking channel in self._led_prot_do, the logic is fragile. For consistency with line 1557, consider using hasattr(self, "protection") before accessing self.protection.value, or document why this is safe.
| if channel in self._led_prot_do and (self.protection.value or self._leds_on): | |
| protection_attr = getattr(self, "protection", None) | |
| protection_active = bool(protection_attr.value) if protection_attr is not None else False | |
| if channel in self._led_prot_do and (protection_active or self._leds_on): |
📝 WalkthroughWalkthroughThis pull request updates SPARC spectrograph acquisition paths and LED protection handling across multiple system components. Changes include propagating default spectrograph parameters to _getAffectingSpectrograph() calls in several spectrum plugins, adding a new PhotoDetectorLivePlugin for live photo-detector spectral display, introducing configuration entries for spectrograph-related timing stream settings, refactoring LED protection logic in the TMCM driver with centralized handling methods, expanding time-correlator mode configuration with slit-in-big actuator mapping, and broadening detector role handling in the metadata updater to include time-correlator and photo-detector types. Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/tmcm.py (1)
1991-2004:⚠️ Potential issue | 🟡 MinorRename unused
lposto avoid lint warnings.🛠️ Proposed fix
- _, hpos, lpos, dur = self._do_axes[channel] + _, hpos, _lpos, dur = self._do_axes[channel]
🤖 Fix all issues with AI agents
In `@plugins/photo_det_live.py`:
- Around line 33-59: Add reST docstrings and return type hints: add a
class-level reST docstring to PhotoDetectorLivePlugin describing purpose and
authorship; add an __init__ docstring in reST and annotate __init__ with ->
None; replace the single-line docstring in add_photo_det_stream with a proper
multi-line reST docstring describing parameters (name, detector), behavior and
return value, and add a return type hint for add_photo_det_stream matching the
actual returned object (the variable stream_cont) — if the concrete type isn't
available, import typing.Any or typing.Optional and annotate accordingly; update
imports if needed.
In `@src/odemis/driver/tmcm.py`:
- Around line 1462-1510: Add a docstring to the _set_protection method
describing its purpose, parameters, and return value (it toggles protection
state, takes protected: bool, returns the new state) and remove the unused lpos
local variable from the _switch_led_prot method where _do_axes is unpacked
(change "do_an, hpos, lpos, dur = self._do_axes[channel]" to only unpack the
used names or use a throwaway name like _ for the unused element). Ensure
references to _refswitch_lock and calls to _switch_led_prot remain unchanged.
In `@src/odemis/odemisd/mdupdater.py`:
- Around line 430-433: The method observeSpectrograph is missing type
annotations and an reST docstring and should be annotated to return
Optional[bool] (it returns False in one branch and implicitly None in others);
add parameter type hints for spectrograph and comp_affected, annotate the nested
function updateOutWLRange if desired, and import Optional from typing. Add a
reST-style docstring for observeSpectrograph describing parameters and the
Optional[bool] return value, and update any signatures that reference
observeSpectrograph/updateOutWLRange to use the new annotations.
| class PhotoDetectorLivePlugin(Plugin): | ||
| name = "Photo-detector live display" | ||
| __version__ = "1.0" | ||
| __author__ = "Éric Piel" | ||
| __license__ = "GPLv2" | ||
|
|
||
| def __init__(self, microscope: model.Microscope, main_app: OdemisGUIApp): | ||
| super().__init__(microscope, main_app) | ||
| main_data = self.main_app.main_data | ||
| if not (microscope and main_data.photo_ds and main_data.role.startswith("sparc")): | ||
| logging.info("%s plugin cannot load as the microscope is not a SPARC with a photo detector.", | ||
| self.name) | ||
| return | ||
|
|
||
| self._tab = self.main_app.main_data.getTabByName("sparc_acqui") | ||
| stctrl = self._tab.streambar_controller | ||
| for det in main_data.photo_ds: | ||
| name = f"{det.name} alignment" | ||
| act = functools.partial(self.add_photo_det_stream, name=name, detector=det) | ||
| stctrl.add_action(name, act) | ||
|
|
||
| # Note: no need to explicitly add the "Temporal Intensity" viewport, because it's normally | ||
| # always created when a time-correlator is available, which is the assumption here. | ||
|
|
||
| def add_photo_det_stream(self, name: str, detector: model.Detector): | ||
| """ Create a Monochromator stream, using a photo-detector and add to to all compatible viewports""" | ||
| main_data = self.main_app.main_data |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the file
find . -name "photo_det_live.py" -type fRepository: delmic/odemis
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Get file size to determine how to read it
wc -l plugins/photo_det_live.pyRepository: delmic/odemis
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Read the relevant lines (33-80 to see full context)
sed -n '33,80p' plugins/photo_det_live.pyRepository: delmic/odemis
Length of output: 2272
🏁 Script executed:
#!/bin/bash
# Search for StreamController class definition and usage
rg "class StreamController" --type py -A 5Repository: delmic/odemis
Length of output: 462
🏁 Script executed:
#!/bin/bash
# Search for add_photo_det_stream method to see what it returns
rg "def add_photo_det_stream" --type py -A 20Repository: delmic/odemis
Length of output: 1618
🏁 Script executed:
#!/bin/bash
# Read the complete add_photo_det_stream method (lines 59-99)
sed -n '59,99p' plugins/photo_det_live.pyRepository: delmic/odemis
Length of output: 1670
Add class and method docstrings in reST format and return type hints.
The class and both methods lack proper documentation and return type annotations per Python guidelines:
- Class
PhotoDetectorLivePluginhas no docstring __init__is missing both docstring and return type hint (-> None)add_photo_det_streamhas an improperly formatted docstring (single-line, non-reST) and is missing a return type hint; the method returnsstream_conton line 97
✏️ Suggested update
class PhotoDetectorLivePlugin(Plugin):
+ """
+ Live-display plugin for photo-detector monochromator streams.
+ """
name = "Photo-detector live display"
@@
- def __init__(self, microscope: model.Microscope, main_app: OdemisGUIApp):
+ def __init__(self, microscope: model.Microscope, main_app: OdemisGUIApp) -> None:
+ """
+ Initialize the plugin and register photo-detector stream actions.
+
+ :param microscope: Active microscope instance.
+ :param main_app: Main GUI application.
+ """
@@
- def add_photo_det_stream(self, name: str, detector: model.Detector):
- """ Create a Monochromator stream, using a photo-detector and add to to all compatible viewports"""
+ def add_photo_det_stream(self, name: str, detector: model.Detector) -> "StreamController":
+ """
+ Create a monochromator stream using a photo-detector and add it to compatible viewports.
+
+ :param name: Stream name.
+ :param detector: Photo-detector to bind.
+ :return: The created stream controller.
+ """🤖 Prompt for AI Agents
In `@plugins/photo_det_live.py` around lines 33 - 59, Add reST docstrings and
return type hints: add a class-level reST docstring to PhotoDetectorLivePlugin
describing purpose and authorship; add an __init__ docstring in reST and
annotate __init__ with -> None; replace the single-line docstring in
add_photo_det_stream with a proper multi-line reST docstring describing
parameters (name, detector), behavior and return value, and add a return type
hint for add_photo_det_stream matching the actual returned object (the variable
stream_cont) — if the concrete type isn't available, import typing.Any or
typing.Optional and annotate accordingly; update imports if needed.
| def _switch_led_prot(self, protected: bool) -> None: | ||
| """ | ||
| Blocks until the move duration is passed, and will update .position based on the actual | ||
| state of the DO axes. | ||
| Must be called with _refswitch_lock held. | ||
| :param protected: If True, force the shutters closed. If False, set them | ||
| to the expected position (if _leds_on is False). | ||
| """ | ||
| tsleep = 0 # max transition period for all shutters | ||
| if protected: | ||
| logging.debug("Forcing the protection active") | ||
| for channel, val in self._led_prot_do.items(): | ||
| do_an, hpos, lpos, dur = self._do_axes[channel] | ||
| # If the shutter is already in the right position, no need to wait for it to move | ||
| if self.position.value[do_an] == val: | ||
| logging.debug("Shutter on axis %s already in protected position", do_an) | ||
| else: | ||
| tsleep = max(tsleep, dur) | ||
| # Set the DO to the "protected" position even if the position reports it's already | ||
| # there to be really sure. It's very fast anyway. | ||
| self.SetIO(2, channel, val == hpos) | ||
| else: | ||
| # Set the shutter in the "right" position: | ||
| # if _leds_on is False, set to the expected position | ||
| # if _leds_on is True, leave them closed (ie, do nothing) | ||
| if not self._leds_on: | ||
| # Set digital axis outputs to latest requested value | ||
| for an, val in self._expected_do_pos.items(): | ||
| channel = self._name_to_do_axis[an] | ||
| if channel not in self._led_prot_do: | ||
| continue | ||
| _, hpos, lpos, dur = self._do_axes[channel] | ||
| if self.position.value[an] == val: | ||
| logging.debug("Shutter on axis %s already in protected position", an) | ||
| else: | ||
| tsleep = max(tsleep, dur) | ||
| self.SetIO(2, channel, val == hpos) | ||
|
|
||
| time.sleep(tsleep) | ||
| self._updatePosition(axes={}) | ||
|
|
||
| def _set_protection(self, protected: bool) -> bool: | ||
| if self.protection.value == protected: | ||
| return protected # no change | ||
|
|
||
| with self._refswitch_lock: | ||
| self._switch_led_prot(protected) | ||
|
|
||
| return protected |
There was a problem hiding this comment.
Add a docstring for _set_protection and drop unused lpos.
_set_protection needs a docstring per guidelines, and lpos is unused here.
🛠️ Proposed fix
for an, val in self._expected_do_pos.items():
channel = self._name_to_do_axis[an]
if channel not in self._led_prot_do:
continue
- _, hpos, lpos, dur = self._do_axes[channel]
+ _, hpos, _lpos, dur = self._do_axes[channel]
if self.position.value[an] == val:
logging.debug("Shutter on axis %s already in protected position", an)
else:
tsleep = max(tsleep, dur)
self.SetIO(2, channel, val == hpos)
@@
- def _set_protection(self, protected: bool) -> bool:
+ def _set_protection(self, protected: bool) -> bool:
+ """
+ Toggle LED protection state.
+
+ :param protected: True to force protection, False to restore expected DO state.
+ :return: The requested protection state.
+ """
if self.protection.value == protected:
return protected # no change🧰 Tools
🪛 Ruff (0.14.14)
[warning] 1493-1493: Unpacked variable lpos is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In `@src/odemis/driver/tmcm.py` around lines 1462 - 1510, Add a docstring to the
_set_protection method describing its purpose, parameters, and return value (it
toggles protection state, takes protected: bool, returns the new state) and
remove the unused lpos local variable from the _switch_led_prot method where
_do_axes is unpacked (change "do_an, hpos, lpos, dur = self._do_axes[channel]"
to only unpack the used names or use a throwaway name like _ for the unused
element). Ensure references to _refswitch_lock and calls to _switch_led_prot
remain unchanged.
| def observeSpectrograph(self, spectrograph, comp_affected): | ||
|
|
||
| if comp_affected.role == "monochromator": | ||
| if any(comp_affected.role.startswith(r) for r in ("monochromator", "time-correlator", "photo-detector")): | ||
| def updateOutWLRange(pos, sp=spectrograph, comp_affected=comp_affected): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/odemis/odemisd/mdupdater.py | sed -n '425,480p'Repository: delmic/odemis
Length of output: 3715
🏁 Script executed:
head -50 src/odemis/odemisd/mdupdater.py | grep -E "^import|^from"Repository: delmic/odemis
Length of output: 192
🏁 Script executed:
rg -n "class.*Updater" src/odemis/odemisd/mdupdater.py -A 5Repository: delmic/odemis
Length of output: 308
🏁 Script executed:
rg -n "def " src/odemis/odemisd/mdupdater.py -B 1 | head -60Repository: delmic/odemis
Length of output: 2220
🏁 Script executed:
sed -n '310,390p' src/odemis/odemisd/mdupdater.pyRepository: delmic/odemis
Length of output: 4223
🏁 Script executed:
sed -n '154,180p' src/odemis/odemisd/mdupdater.pyRepository: delmic/odemis
Length of output: 1297
🏁 Script executed:
sed -n '178,290p' src/odemis/odemisd/mdupdater.pyRepository: delmic/odemis
Length of output: 5807
🏁 Script executed:
sed -n '484,510p' src/odemis/odemisd/mdupdater.pyRepository: delmic/odemis
Length of output: 1674
Add type hints and a reST docstring for observeSpectrograph.
This method lacks annotations and a reST docstring required by the project's Python guidelines. However, the suggested return type should be Optional[bool] instead of bool, since the method returns False in the else branch or None implicitly in the if/elif branches.
✏️ Suggested update
- def observeSpectrograph(self, spectrograph, comp_affected):
+ def observeSpectrograph(
+ self,
+ spectrograph: model.HwComponent,
+ comp_affected: model.HwComponent,
+ ) -> Optional[bool]:
+ """
+ Observe spectrograph changes and update the affected component metadata.
+ :param spectrograph: Spectrograph component.
+ :param comp_affected: Component affected by the spectrograph.
+ :return: False if the component role is not supported, None otherwise.
+ """📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def observeSpectrograph(self, spectrograph, comp_affected): | |
| if comp_affected.role == "monochromator": | |
| if any(comp_affected.role.startswith(r) for r in ("monochromator", "time-correlator", "photo-detector")): | |
| def updateOutWLRange(pos, sp=spectrograph, comp_affected=comp_affected): | |
| def observeSpectrograph( | |
| self, | |
| spectrograph: model.HwComponent, | |
| comp_affected: model.HwComponent, | |
| ) -> Optional[bool]: | |
| """ | |
| Observe spectrograph changes and update the affected component metadata. | |
| :param spectrograph: Spectrograph component. | |
| :param comp_affected: Component affected by the spectrograph. | |
| :return: False if the component role is not supported, None otherwise. | |
| """ | |
| if any(comp_affected.role.startswith(r) for r in ("monochromator", "time-correlator", "photo-detector")): | |
| def updateOutWLRange(pos, sp=spectrograph, comp_affected=comp_affected): |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 433-433: Unused function argument: pos
(ARG001)
🤖 Prompt for AI Agents
In `@src/odemis/odemisd/mdupdater.py` around lines 430 - 433, The method
observeSpectrograph is missing type annotations and an reST docstring and should
be annotated to return Optional[bool] (it returns False in one branch and
implicitly None in others); add parameter type hints for spectrograph and
comp_affected, annotate the nested function updateOutWLRange if desired, and
import Optional from typing. Add a reST-style docstring for observeSpectrograph
describing parameters and the Optional[bool] return value, and update any
signatures that reference observeSpectrograph/updateOutWLRange to use the new
annotations.
1 participant
Add support for placing the optical fiber going to the LabCube after spectrograph instead of before. That means that the labcube now receives a very narrow band of the light wavelength. IOW, this allows time correlator acquisitions wavelength specific.
In practice, this means this changes: