[TRPD-29][feat] SPARC acq/GUI: support for streakcam photon-counting - #3543
[TRPD-29][feat] SPARC acq/GUI: support for streakcam photon-counting#3543pieleric wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughPhoton-counting mode now uses dedicated exposure and integration-count settings in acquisition timing, CCD preparation, readout estimation, and hardware synchronization. Streak-camera intensity protection is disabled in this mode. The GUI exposes photon-counting controls and hides standard exposure controls when enabled. Streak alignment disables photon counting. Tests cover updated integration timing and repeated photon-counting acquisitions. Sequence Diagram(s)sequenceDiagram
participant StreamController
participant SEMCCD
participant StreakCCD
StreamController->>SEMCCD: select photon-counting settings
SEMCCD->>StreakCCD: configure exposure and integration count
StreakCCD-->>SEMCCD: return one integrated frame
SEMCCD-->>StreamController: provide acquisition metadata and image
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds photon-counting support for SPARC streak camera Temporal Spectrum acquisition and the corresponding GUI controls, including updated acquisition-time estimation and dedicated test coverage for photon-counting mode.
Changes:
- Add GUI logic to show/hide exposure/integration controls based on photon-counting mode for Temporal Spectrum streams.
- Extend acquisition timing and hardware-preparation paths to account for photon-counting exposure/count parameters.
- Update streakcam stream tests and add a dedicated photon-counting acquisition test.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/gui/cont/stream.py | Toggle visibility of exposure/integration controls based on photon-counting mode. |
| src/odemis/gui/cont/stream_bar.py | Add notes/TODO around passing photon-counting VAs for better GUI ordering. |
| src/odemis/gui/conf/data.py | Add photon-counting-related VAs to streak-CCD GUI config and refine tooltips. |
| src/odemis/gui/comp/stream_panel.py | Adjust GridBagSizer empty-cell size to improve layout behavior. |
| src/odemis/acq/stream/test/stream_sparc2_streakcam_test.py | Stabilize/update integration-related assertions and add photon-counting acquisition test. |
| src/odemis/acq/stream/_sync.py | Add photon-counting branches for timing/protection/hardware prep; update acquisition logging. |
| src/odemis/acq/stream/_helper.py | Update acquisition-time estimate to incorporate photon-counting exposure/count parameters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a431564 to
26acfc5
Compare
pieleric
changed the title
If the streak-cam supports photon-counting, show the option, and when the option is active, only show the related settings.
26acfc5 to
da18b63
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/acq/stream/_sync.py (1)
1000-1017: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a per-integration duration before applying
counts.For ordinary integration,
integrationTimeis already the total requested duration. Lines 1003-1005 multiply that total byintegrationCountsagain at line 1017.For hardware synchronization in photon-counting mode, lines 1010-1013 omit
detPcIntegrationCounts. A 10 ms exposure with 100 counts is estimated as about one 10 ms frame instead of about one second plus readout. The estimated timeout can then expire before acquisition completes.Proposed fix
elif self._integrationTime: - exp = self._integrationTime.value # get the total exp time counts = self._integrationCounts.value + exp = self._integrationTime.value / counts else: exp = self._sccd._getDetectorVA("exposureTime").value counts = 1 if self._supports_hw_sync(): # The overhead per frame depends a lot on the camera. For now, we use arbitrarily the # overhead observed on an Andor Newton (8 ms). - dur_image = exp + readout + 0.008 + dur_image = (exp + readout) * counts + 0.008🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/stream/_sync.py` around lines 1000 - 1017, Update the duration calculation around _supports_hw_sync so exp represents per-integration exposure before counts are applied: preserve the ordinary _integrationTime total duration without multiplying it by _integrationCounts again, while incorporating detPcIntegrationCounts for hardware-synchronized photon-counting acquisitions. Ensure dur_image reflects the full integration duration plus the appropriate readout and existing overheads in both branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/acq/stream/test/stream_sparc2_streakcam_test.py`:
- Line 866: Update the roi_to_phys call in the streaks test to bind only the
required resolution return value, discarding the unused position and pixel
results while preserving the existing exp_res usage.
- Line 838: Update the test_streak_acq_photon_counting method signature to
include the -> None return type annotation, preserving its existing parameters
and behavior.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 1985-1989: The Temporal Spectrum stream must keep photon-counting
state synchronized with alignment changes. In src/odemis/gui/cont/stream_bar.py
lines 1985-1989, update the Temporal Spectrum stream setup to expose
pcExposureTime and pcIntegrationCounts as synchronized hardware VAs, or provide
an explicit synchronization path for existing streams so detPhotonCounting and
related values cannot remain stale. In
src/odemis/gui/cont/tabs/sparc2_align_tab.py lines 1337-1340, update the
alignment flow to disable photon counting through that shared synchronized state
before starting alignment.
In `@src/odemis/gui/cont/stream.py`:
- Around line 554-564: Update the docstring for the photon-counting handler to
replace the reStructuredText “:param active:” field with a plain-text sentence
describing that active indicates whether photon-counting mode is enabled.
Preserve the existing behavior and other documentation.
- Around line 217-220: Update the subscription created in the panel
initialization flow around _on_photon_counting so it is explicitly disconnected
when the panel is destroyed. Store or otherwise track the detPhotonCounting
subscription and remove it in the panel’s destruction/cleanup method alongside
existing entry.disconnect() handling, preventing callbacks from retaining
StreamController or accessing destroyed controls.
---
Outside diff comments:
In `@src/odemis/acq/stream/_sync.py`:
- Around line 1000-1017: Update the duration calculation around
_supports_hw_sync so exp represents per-integration exposure before counts are
applied: preserve the ordinary _integrationTime total duration without
multiplying it by _integrationCounts again, while incorporating
detPcIntegrationCounts for hardware-synchronized photon-counting acquisitions.
Ensure dur_image reflects the full integration duration plus the appropriate
readout and existing overheads in both branches.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 164ec30d-0885-46c7-b1ce-af1c33d87237
📒 Files selected for processing (8)
src/odemis/acq/stream/_helper.pysrc/odemis/acq/stream/_sync.pysrc/odemis/acq/stream/test/stream_sparc2_streakcam_test.pysrc/odemis/gui/comp/stream_panel.pysrc/odemis/gui/conf/data.pysrc/odemis/gui/cont/stream.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.py
| temporalSpectrum_drift = ts_da[-1] # drift correction image | ||
| self.assertGreaterEqual(temporalSpectrum_drift.shape[-4], 2) | ||
|
|
||
| def test_streak_acq_photon_counting(self): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the return type annotation.
Line 838 defines a new test method without a return annotation. Add -> None.
Proposed fix
- def test_streak_acq_photon_counting(self):
+ def test_streak_acq_photon_counting(self) -> None:As per coding guidelines, “Always use type hints for function parameters and return types in Python code”.
📝 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 test_streak_acq_photon_counting(self): | |
| def test_streak_acq_photon_counting(self) -> None: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/acq/stream/test/stream_sparc2_streakcam_test.py` at line 838,
Update the test_streak_acq_photon_counting method signature to include the ->
None return type annotation, preserving its existing parameters and behavior.
Source: Coding guidelines
|
|
||
| streaks.repetition.value = (7, 3) | ||
| num_ts = numpy.prod(streaks.repetition.value) # number of expected temporal spectrum images | ||
| exp_pos, exp_pxs, exp_res = roi_to_phys(streaks) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Discard the unused return values.
Line 866 does not use exp_pos or exp_pxs. Bind only the required resolution.
Proposed fix
- exp_pos, exp_pxs, exp_res = roi_to_phys(streaks)
+ _, _, exp_res = roi_to_phys(streaks)Static analysis reports these bindings as unused.
📝 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.
| exp_pos, exp_pxs, exp_res = roi_to_phys(streaks) | |
| _, _, exp_res = roi_to_phys(streaks) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 866-866: Unpacked variable exp_pos is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 866-866: Unpacked variable exp_pxs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/acq/stream/test/stream_sparc2_streakcam_test.py` at line 866,
Update the roi_to_phys call in the streaks test to bind only the required
resolution return value, discarding the unused position and pixel results while
preserving the existing exp_res usage.
Source: Linters/SAST tools
| # TODO: ideally, pcExposureTime and pcIntegrationCounts should be passed as hwdetvas, so that | ||
| # they get immediately updated when the user changes them in HPDTA. However, currently, the | ||
| # StreamController shows the hwdetvas before the other VAs, which prevents them from being | ||
| # displayed next to the photon-counting checkbox. Once StreamController is adjusted to show | ||
| # every VAs according to the STREAM_SETTINGS_CONFIG, this can be changed. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep photon-counting state synchronized across acquisition and alignment.
The Temporal Spectrum stream stores photon-counting VAs locally. Alignment writes main.streak_ccd.photonCounting directly. The existing stream can then retain stale detPhotonCounting and photon-counting values after alignment disables the hardware mode.
src/odemis/gui/cont/stream_bar.py#L1985-L1989: expose photon-counting VAs as synchronized hardware VAs, or add an explicit synchronization path for existing Temporal Spectrum streams.src/odemis/gui/cont/tabs/sparc2_align_tab.py#L1337-L1340: disable photon counting through the shared synchronized state before starting alignment.
📍 Affects 2 files
src/odemis/gui/cont/stream_bar.py#L1985-L1989(this comment)src/odemis/gui/cont/tabs/sparc2_align_tab.py#L1337-L1340
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/cont/stream_bar.py` around lines 1985 - 1989, The Temporal
Spectrum stream must keep photon-counting state synchronized with alignment
changes. In src/odemis/gui/cont/stream_bar.py lines 1985-1989, update the
Temporal Spectrum stream setup to expose pcExposureTime and pcIntegrationCounts
as synchronized hardware VAs, or provide an explicit synchronization path for
existing streams so detPhotonCounting and related values cannot remain stale. In
src/odemis/gui/cont/tabs/sparc2_align_tab.py lines 1337-1340, update the
alignment flow to disable photon counting through that shared synchronized state
before starting alignment.
| # For Temporal Spectrum streams, with a photon counting mode, show/hide the exposure time controls | ||
| if hasattr(stream, "detPhotonCounting"): | ||
| self.stream.detPhotonCounting.subscribe(self._on_photon_counting, init=True) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unsubscribe from detPhotonCounting when the panel is destroyed.
This direct subscription is not removed by entry.disconnect(). It retains StreamController after panel destruction. A later callback can access destroyed controls.
Proposed fix
def _on_stream_panel_destroy(self):
+ if hasattr(self.stream, "detPhotonCounting"):
+ self.stream.detPhotonCounting.unsubscribe(self._on_photon_counting)
+
self._unlink_resolution()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/cont/stream.py` around lines 217 - 220, Update the
subscription created in the panel initialization flow around _on_photon_counting
so it is explicitly disconnected when the panel is destroyed. Store or otherwise
track the detPhotonCounting subscription and remove it in the panel’s
destruction/cleanup method alongside existing entry.disconnect() handling,
preventing callbacks from retaining StreamController or accessing destroyed
controls.
| """ | ||
| For Temporal Spectrum streams, with a photon counting mode, show/hide the exposure time controls. | ||
| Photon-counting disabled: | ||
| - integrationTime/exposureTime | ||
| - integrationCounts | ||
| Photon-counting enabled: | ||
| - (pc)IntegrationCounts | ||
| - (pc)ExposureTime | ||
| Called when the .photonCounting VA is changed | ||
| :param active: True if Photon counting mode is enabled | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a plain-text docstring.
Replace the :param: field with a plain-text sentence. Based on learnings, docstrings must be plain text and must not use reStructuredText directives such as :param:.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/cont/stream.py` around lines 554 - 564, Update the docstring
for the photon-counting handler to replace the reStructuredText “:param active:”
field with a plain-text sentence describing that active indicates whether
photon-counting mode is enabled. Preserve the existing behavior and other
documentation.
Source: Learnings
Extend the GUI to display photon-counting option on the Temporal Spectrum streams, if the streak-cam supports it.
When the option is active, only show the related settings (exposure time per frame + frame count)
Also extend the acquisition code to support it. In practice, it's mostly just about reading the right settings to predict correctly the acquisition duration.