Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a spectral peak alignment workflow for grating–detector offset calibration. A new module implements gaussian modeling, peak finding, goffset scale estimation, and an asynchronous iterative alignment task (sparc_auto_grating_offset). A separate orchestrator provides multi-grating/multi-detector alignment (auto_align_grating_detector_offsets) with selector support, cancellation, progress reporting, and time estimates. New tests exercise unit and end-to-end behaviors, including cancellation. Sequence Diagram(s)sequenceDiagram
participant Client
participant API as sparc_auto_grating_offset
participant Task as _do_sparc_auto_grating_offset
participant Spectrograph
participant Detector
Client->>API: invoke alignment(tolerance_px,max_it)
API->>API: create ProgressiveFuture
API->>Task: start background task
API-->>Client: return ProgressiveFuture
loop while error > tolerance
Task->>Detector: acquire spectrum
Task->>Task: find_peak_position()
Task->>Task: compute error from center
Task->>Spectrograph: adjust goffset (bounded proportional step)
Task->>Task: update progress / check cancellation
alt Cancelled
Task->>Client: raise CancelledError
end
end
alt converged
Task->>API: set result (final pos)
else max iterations
Task->>API: set last pos, log warning
end
API->>API: mark FINISHED
sequenceDiagram
participant Client
participant API as auto_align_grating_detector_offsets
participant Orchestrator as _do_auto_align_grating_detector_offsets
participant sparc as sparc_auto_grating_offset
participant Selector
participant Spectrograph
Client->>API: call(spectrograph, detectors, selector?)
API->>API: validate inputs, create ProgressiveFuture
API->>Orchestrator: start background task
API-->>Client: return ProgressiveFuture
rect rgba(100,150,200,0.5)
Note over Orchestrator: Stage 1 — align first grating across all detectors
loop for each detector
alt selector provided
Orchestrator->>Selector: switch to detector's selector state
end
Orchestrator->>sparc: align current grating (subfuture)
sparc-->>Orchestrator: return result
end
end
rect rgba(200,150,100,0.5)
Note over Orchestrator: Stage 2 — align remaining gratings with first detector
loop for each remaining grating
Orchestrator->>sparc: align grating
sparc-->>Orchestrator: return result
end
end
Orchestrator->>Selector: restore original selector state
Orchestrator->>Spectrograph: restore original position
Orchestrator->>API: set aggregated results
API->>API: mark FINISHED
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ 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 |
pieleric
requested changes
Mar 6, 2026
Member
There was a problem hiding this comment.
Please move all the content of this file into goffset.py . No need to have separate files, as all the functions are related.
src/odemis/acq/align/goffset.py
Outdated
|
|
||
| return f | ||
|
|
||
| def _DoSparcAutoGratingOffset(future: model.ProgressiveFuture, |
Member
There was a problem hiding this comment.
For new code, we now use for the function names "snake_case" everywhere. So could you rename all the functions to use this (new) convention. I know, we suggested you to model your code to autofocus.py, sorry!
src/odemis/acq/align/goffset.py
Outdated
|
|
||
| success = False | ||
|
|
||
| logging.info("Running alignment | detector=%s | align_grating=%s",detector.name, align_grating,) |
Member
There was a problem hiding this comment.
Please fix the format: space after , and no , after the last parameter.
You can use autopep8, which should be able to hint you on everywhere the style is incorrect.
src/odemis/acq/align/goffset.py
Outdated
| delta_goffset = max(-max_step, min(max_step, delta_goffset)) | ||
| total_goffset_displacement += delta_goffset | ||
|
|
||
| print(f"DEBUG | Iter: {i} | Peak: {peak_px:.1f} | Error: {error_px:.1f} | Move: {delta_goffset:.4f} | Total Change: {total_goffset_displacement:.4f}") |
Member
There was a problem hiding this comment.
No print() in the code. You can use logging.debug()
src/odemis/acq/align/goffset.py
Outdated
|
|
||
| print(f"DEBUG | Iter: {i} | Peak: {peak_px:.1f} | Error: {error_px:.1f} | Move: {delta_goffset:.4f} | Total Change: {total_goffset_displacement:.4f}") | ||
| spgr.moveRelSync({"goffset": delta_goffset}) | ||
| time.sleep(2) |
Member
There was a problem hiding this comment.
I think we had added this for debugging when the simulator was too fast, right? In general, you should never need a time.sleep().
| peak = popt[1] | ||
|
|
||
| if start <= peak <= end: | ||
| return float(peak) |
Member
There was a problem hiding this comment.
It'd be helpful to have a logging in case of the else, so that we know something probably went wrong.
src/odemis/acq/align/goffset.py
Outdated
|
|
||
| def estimate_goffset_scale(spgr: model.Actuator, detector: model.Detector, delta=5.0) -> float: | ||
| """ | ||
| Estimates how many pixels the peak shifts per 1 unit of goffset. |
Member
There was a problem hiding this comment.
extend the doctring to document params and return value.
src/odemis/acq/align/goffset.py
Outdated
| logging.info( | ||
| f"SCALE TRACKING | p0: {p0:.1f} | p1: {p1:.1f} | Delta: {actual_delta} | Shift: {(p1-p0):.1f} | Result Scale: {scale:.4f}") | ||
|
|
||
| if abs(scale) < 1e-3: |
Member
There was a problem hiding this comment.
Recursive functions are tricky to ensure they always end. Looking at the code, it's not obviously clear how you can ensure this. Can you add a comment to explain?
src/odemis/acq/align/goffset.py
Outdated
|
|
||
| return scale | ||
|
|
||
| def SparcAutoGratingOffset(spgr: model.Actuator, |
src/odemis/acq/align/goffset.py
Outdated
| logging.info("Running alignment | detector=%s | align_grating=%s",detector.name, align_grating,) | ||
|
|
||
| try: | ||
| scale = estimate_goffset_scale(spgr, detector) |
Member
There was a problem hiding this comment.
As an optimisation, you could use the images acquired during this step, to already measure the position of the peak at the current offset (and a few other offsets too). This would speed up the first iterations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
src/odemis/acq/align/goffset_alignment.py (1)
43-46:⚠️ Potential issue | 🟠 MajorRemove or properly type
streams; right now it is a dead, unresolved API parameter.
Streamis not imported anywhere, andstreamsis never used in either the public entry point or the worker. That leaves the signature with a lint-blocking unresolved annotation and an argument callers may assume matters.Also applies to: 75-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset_alignment.py` around lines 43 - 46, The parameter streams on auto_align_grating_detector_offsets is an unused, unresolved type (Stream not imported) and should be removed or properly typed and threaded through; update the signature of auto_align_grating_detector_offsets (and the corresponding worker/public entry point functions mentioned around lines 75-80) to either (a) drop the streams parameter everywhere it’s declared and called, or (b) import or declare the Stream type and pass streams into the worker flow so it’s actually used; ensure you update all call sites and any type annotations for model.ProgressiveFuture, the worker function, and selector/detectors handling to keep the API and types consistent.src/odemis/acq/align/test/goffset_test.py (1)
106-117:⚠️ Potential issue | 🟠 Major
test_cancelis masking failures instead of testing cancellation.The body never calls
f.cancel(), and the bareexcept: passhides timeouts and real alignment errors alike. This can go green without proving any cancellation behavior. Assertcancel()/cancelled()and catch only the expected exception.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 106 - 117, The test_cancel test is masking failures and never actually tests cancellation: modify test_cancel to call cancel() on the SparcAutoGratingOffset future (f.cancel()), assert that the cancel call returned True (or that f.cancelled() becomes True), and then call f.result() expecting the appropriate cancellation exception (catch only concurrent.futures.CancelledError or the specific exception your future raises) instead of a bare except; finally keep the existing f.done() assertion to ensure the future finished. Use the symbols test_cancel, SparcAutoGratingOffset, f.cancel(), f.cancelled(), f.result(), and f.done() to locate and update the logic.src/odemis/acq/align/test/goffset_alignment_test.py (1)
118-122:⚠️ Potential issue | 🟠 MajorFail the test when
sp-ccdreturns a flat image.Printing a warning here lets the test pass on the exact failure mode this post-alignment check is trying to detect. Turn this into an assertion and send any extra diagnostics through
logging.debug()instead ofprint().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_alignment_test.py` around lines 118 - 122, The check that currently prints a warning when sp-ccd returns a flat image should be turned into a failing assertion: replace the print-based conditional around `data.max() == data.min()` with an assertion such as `assert data.max() != data.min(), "sp-ccd returned a flat image"` and move any extra diagnostics into `logging.debug()` calls (e.g., log the min/max and any context), ensuring `logging` is imported or configured in the test file and removing the `print()`s so the test fails on the flat-image condition.src/odemis/acq/align/goffset.py (1)
151-157:⚠️ Potential issue | 🔴 CriticalBound the retry path before this recurses indefinitely.
If the measured scale stays outside the accepted range but no
RuntimeErroris raised, this callsestimate_goffset_scale()again with the same inputs and no retry budget. That can recurse untilRecursionErrorwhile repeatedly moving the hardware.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 151 - 157, The current logic can recurse indefinitely when estimate_goffset_scale(spgr, detector) returns a scale outside the accepted range but does not raise; replace the implicit recursion with a bounded retry loop: implement a max_attempts constant (e.g., 3–5), call estimate_goffset_scale(spgr, detector) inside the loop, break when the returned scale is within [1e-3, 10.0], and on RuntimeError or exhausting attempts log a warning and set scale to the safe default (0.5); update references in this block to use the looped variables scale, p0, p1 and ensure no recursive call to estimate_goffset_scale remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/align/goffset_alignment.py`:
- Around line 17-26: The function _checkCancelled currently snapshots state
before acquiring the mutex, allowing a race where cancellation can be missed;
fix it by resolving lock_name and state_name as you do now, fetch the lock via
getattr(future, lock_name) first, then enter with lock: and inside the
with-block read state = getattr(future, state_name) and raise CancelledError()
if state == CANCELLED (preserving lock acquisition around the state read).
Ensure you still compute lock_name/state_name using hasattr as before and
reference CANCELLED and CancelledError exactly as in the current code.
- Around line 116-124: The loop currently skips calling
sparc_auto_grating_offset and recording results when selector is None; change
the logic in auto_align_grating_detector_offsets so
selector.moveAbsSync({selector_axes: detector_to_selector[d]}) is executed only
if selector exists, but sparc_auto_grating_offset(spectrograph, d) is always
invoked (set future._subfuture = sparc_auto_grating_offset(...)), await/result
it, and write results[(g0, d.name)] = success regardless of selector presence;
keep logging and _checkCancelled(future) as-is and reuse detector_to_selector
only when selector is not None.
In `@src/odemis/acq/align/goffset.py`:
- Around line 214-216: The code uses detector.width/2 for center_target but
find_peak_position() returns zero-based pixel coords, so change center_target to
use (detector.resolution.value[0] - 1) / 2 (i.e., midpoint in the same
zero-based coordinate system) where center_target is computed in
estimate_goffset_scale-related logic (currently set in the block using
estimate_goffset_scale and the subsequent block around lines 234-243); update
all other places that compute target midpoint the same way so the peak fit and
detector midpoint share the same coordinate system.
- Around line 65-75: The centroid calculation uses window_data instead of the
clipped positive weights, so replace the numerator/denominator in the else
branch to use weights (i.e., use numpy.sum(window_idx * weights) /
numpy.sum(weights)) to compute weighted_avg; keep the fallback that sets
weighted_avg = float(peak_idx) when weights.sum() == 0 and retain the logging
call referencing peak_idx.
- Around line 96-117: The function estimate_goffset_scale currently annotates
its return as -> float but actually returns (scale, p0, p1); update the
signature to -> Tuple[float, float, float] and import Tuple from typing (or
typing import tuple alias) so type checkers match runtime behavior, and make the
docstring explicitly state the tuple return; alternatively, if you prefer to
return only the scale, change estimate_goffset_scale to return just scale and
update all call sites that unpack three values to use only the single returned
value and adjust the docstring accordingly — pick one approach and ensure
function name estimate_goffset_scale and its callers are updated consistently.
In `@src/odemis/acq/align/test/goffset_alignment_test.py`:
- Around line 69-79: The test suite defines two methods named test_cancel so the
later one overwrites the earlier; rename one of them (e.g., rename the earlier
test_cancel to test_cancel_before_start or test_cancel_after_delay) or merge
their assertions into a single test to cover both cancellation scenarios; update
references to the future returned by auto_align_grating_detector_offsets (f),
its cancel() and cancelled() calls, and the CancelledError expectation on
f.result(...) so both scenarios are executed and collected by the test runner.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 44-50: Uncomment and re-enable the class teardown so the
spectrograph state is always restored: implement/restore the `@classmethod`
tearDownClass that calls cls.spgr.moveAbsSync(cls._original_position) inside a
try/except and logs failures with logging.exception("Failed restoring
spectrograph position"), ensuring the test suite saved cls._original_position
(e.g., in setUpClass) before moving the device so cleanup will reliably return
the backend to its original state.
- Around line 95-100: The test sets delta = 0 so it never introduces any
misalignment; change delta to a non-zero value that is clamped to stay inside
the axis range before calling self.spgr.moveRelSync. Compute a small positive
candidate delta (e.g. 1 step or a fraction) and then clamp it against
goffset_max - current (and a small epsilon) so it won’t exceed the axis, keep
the existing direction calculation using current, goffset_max and direction, and
then call self.spgr.moveRelSync({"goffset": delta * direction}) to actually
create the misalignment the test intends to verify.
---
Duplicate comments:
In `@src/odemis/acq/align/goffset_alignment.py`:
- Around line 43-46: The parameter streams on
auto_align_grating_detector_offsets is an unused, unresolved type (Stream not
imported) and should be removed or properly typed and threaded through; update
the signature of auto_align_grating_detector_offsets (and the corresponding
worker/public entry point functions mentioned around lines 75-80) to either (a)
drop the streams parameter everywhere it’s declared and called, or (b) import or
declare the Stream type and pass streams into the worker flow so it’s actually
used; ensure you update all call sites and any type annotations for
model.ProgressiveFuture, the worker function, and selector/detectors handling to
keep the API and types consistent.
In `@src/odemis/acq/align/goffset.py`:
- Around line 151-157: The current logic can recurse indefinitely when
estimate_goffset_scale(spgr, detector) returns a scale outside the accepted
range but does not raise; replace the implicit recursion with a bounded retry
loop: implement a max_attempts constant (e.g., 3–5), call
estimate_goffset_scale(spgr, detector) inside the loop, break when the returned
scale is within [1e-3, 10.0], and on RuntimeError or exhausting attempts log a
warning and set scale to the safe default (0.5); update references in this block
to use the looped variables scale, p0, p1 and ensure no recursive call to
estimate_goffset_scale remains.
In `@src/odemis/acq/align/test/goffset_alignment_test.py`:
- Around line 118-122: The check that currently prints a warning when sp-ccd
returns a flat image should be turned into a failing assertion: replace the
print-based conditional around `data.max() == data.min()` with an assertion such
as `assert data.max() != data.min(), "sp-ccd returned a flat image"` and move
any extra diagnostics into `logging.debug()` calls (e.g., log the min/max and
any context), ensuring `logging` is imported or configured in the test file and
removing the `print()`s so the test fails on the flat-image condition.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 106-117: The test_cancel test is masking failures and never
actually tests cancellation: modify test_cancel to call cancel() on the
SparcAutoGratingOffset future (f.cancel()), assert that the cancel call returned
True (or that f.cancelled() becomes True), and then call f.result() expecting
the appropriate cancellation exception (catch only
concurrent.futures.CancelledError or the specific exception your future raises)
instead of a bare except; finally keep the existing f.done() assertion to ensure
the future finished. Use the symbols test_cancel, SparcAutoGratingOffset,
f.cancel(), f.cancelled(), f.result(), and f.done() to locate and update the
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09eb8918-bbc1-43a4-9edf-2f7d4f5d883a
📒 Files selected for processing (4)
src/odemis/acq/align/goffset.pysrc/odemis/acq/align/goffset_alignment.pysrc/odemis/acq/align/test/goffset_alignment_test.pysrc/odemis/acq/align/test/goffset_test.py
Comment on lines
+116
to
+124
| for d in detectors_sorted: | ||
| _checkCancelled(future) | ||
| logging.info(f"Starting alignment | Detector: {d.name} | Grating: {g0}") | ||
|
|
||
| if selector: | ||
| selector.moveAbsSync({selector_axes: detector_to_selector[d]}) | ||
| future._subfuture = sparc_auto_grating_offset(spectrograph, d) | ||
| success = future._subfuture.result() | ||
| results[(g0, d.name)] = success |
There was a problem hiding this comment.
Run the first-grating alignment even when selector is None.
auto_align_grating_detector_offsets() explicitly allows the single-detector/no-selector case, but inside this loop the actual sparc_auto_grating_offset() call and result recording are both nested under if selector:. In that valid configuration the first grating is never aligned or stored.
Possible fix
for d in detectors_sorted:
_checkCancelled(future)
logging.info(f"Starting alignment | Detector: {d.name} | Grating: {g0}")
if selector:
selector.moveAbsSync({selector_axes: detector_to_selector[d]})
- future._subfuture = sparc_auto_grating_offset(spectrograph, d)
- success = future._subfuture.result()
- results[(g0, d.name)] = success
-
- logging.info(f"Finished alignment | Detector: {d.name} | Grating: {g0} | Success: {success}")
+ future._subfuture = sparc_auto_grating_offset(spectrograph, d)
+ success = future._subfuture.result()
+ results[(g0, d.name)] = success
+ logging.info(f"Finished alignment | Detector: {d.name} | Grating: {g0} | Success: {success}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/goffset_alignment.py` around lines 116 - 124, The loop
currently skips calling sparc_auto_grating_offset and recording results when
selector is None; change the logic in auto_align_grating_detector_offsets so
selector.moveAbsSync({selector_axes: detector_to_selector[d]}) is executed only
if selector exists, but sparc_auto_grating_offset(spectrograph, d) is always
invoked (set future._subfuture = sparc_auto_grating_offset(...)), await/result
it, and write results[(g0, d.name)] = success regardless of selector presence;
keep logging and _checkCancelled(future) as-is and reuse detector_to_selector
only when selector is not None.
Comment on lines
+65
to
+75
| # calculate the weighted averages | ||
| weights = window_data.clip(min=0) | ||
|
|
||
| # Corner case: window has no positive signal. Use the maximum index itself | ||
| # as the best estimate of the peak, since weighted average would be undefined | ||
| if weights.sum() == 0: | ||
| weighted_avg = float(peak_idx) | ||
| logging.info("Weighted average fallback: all window data <= 0, using peak_idx=%d as estimate", | ||
| peak_idx) | ||
| else: | ||
| weighted_avg = float(numpy.sum(window_idx * window_data) / numpy.sum(window_data)) |
There was a problem hiding this comment.
Use the clipped positive weights in the centroid calculation.
The code checks weights.sum(), but the actual centroid still uses window_data instead of weights. If the window contains negative baseline values, the numerator and denominator can cancel or pull the estimate away from the positive lobe.
Possible fix
- weighted_avg = float(numpy.sum(window_idx * window_data) / numpy.sum(window_data))
+ weighted_avg = float(numpy.sum(window_idx * weights) / numpy.sum(weights))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/goffset.py` around lines 65 - 75, The centroid
calculation uses window_data instead of the clipped positive weights, so replace
the numerator/denominator in the else branch to use weights (i.e., use
numpy.sum(window_idx * weights) / numpy.sum(weights)) to compute weighted_avg;
keep the fallback that sets weighted_avg = float(peak_idx) when weights.sum() ==
0 and retain the logging call referencing peak_idx.
Comment on lines
+214
to
+216
| scale, p0, p1 = estimate_goffset_scale(spgr, detector) | ||
| center_target = detector.resolution.value[0] / 2 # adjust if 0 is not the center | ||
| total_goffset_displacement = 0.0 |
There was a problem hiding this comment.
Target the detector midpoint in the same coordinate system as the peak fit.
find_peak_position() reports zero-based pixel coordinates, so an even-width detector is centered at (width - 1) / 2, not width / 2. With the current target, a perfectly centered peak still reads as 0.5 px off and can miss the 0.4 px tolerance forever.
Also applies to: 234-243
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 214-214: Unpacked variable p1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/goffset.py` around lines 214 - 216, The code uses
detector.width/2 for center_target but find_peak_position() returns zero-based
pixel coords, so change center_target to use (detector.resolution.value[0] - 1)
/ 2 (i.e., midpoint in the same zero-based coordinate system) where
center_target is computed in estimate_goffset_scale-related logic (currently set
in the block using estimate_goffset_scale and the subsequent block around lines
234-243); update all other places that compute target midpoint the same way so
the peak fit and detector midpoint share the same coordinate system.
Comment on lines
+69
to
+79
| def test_cancel(self): | ||
| f = auto_align_grating_detector_offsets(spectrograph=self.spgr, detectors=[self.ccd],) | ||
|
|
||
| time.sleep(1) | ||
|
|
||
| cancelled = f.cancel() | ||
| self.assertTrue(cancelled) | ||
| self.assertTrue(f.cancelled()) | ||
|
|
||
| with self.assertRaises(CancelledError): | ||
| f.result(timeout=900) |
There was a problem hiding this comment.
The second test_cancel replaces the first one.
Python keeps only the later method definition, so the implementation at Lines 40-49 is never collected. Rename or merge one of these tests; otherwise one cancellation scenario disappears silently.
🧰 Tools
🪛 Ruff (0.15.4)
[error] 69-69: Redefinition of unused test_cancel from line 40: test_cancel redefined here
(F811)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/test/goffset_alignment_test.py` around lines 69 - 79,
The test suite defines two methods named test_cancel so the later one overwrites
the earlier; rename one of them (e.g., rename the earlier test_cancel to
test_cancel_before_start or test_cancel_after_delay) or merge their assertions
into a single test to cover both cancellation scenarios; update references to
the future returned by auto_align_grating_detector_offsets (f), its cancel() and
cancelled() calls, and the CancelledError expectation on f.result(...) so both
scenarios are executed and collected by the test runner.
Comment on lines
+44
to
+50
| # @classmethod | ||
| # def tearDownClass(cls): | ||
| # # restore original position | ||
| # try: | ||
| # cls.spgr.moveAbsSync(cls._original_position) | ||
| # except Exception: | ||
| # logging.exception("Failed restoring spectrograph position") |
There was a problem hiding this comment.
Restore the spectrograph state after this suite.
These tests move the spectrograph alignment state, but the cleanup path is commented out. That makes later tests order-dependent and can leave the backend in a modified state after the suite finishes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/test/goffset_test.py` around lines 44 - 50, Uncomment
and re-enable the class teardown so the spectrograph state is always restored:
implement/restore the `@classmethod` tearDownClass that calls
cls.spgr.moveAbsSync(cls._original_position) inside a try/except and logs
failures with logging.exception("Failed restoring spectrograph position"),
ensuring the test suite saved cls._original_position (e.g., in setUpClass)
before moving the device so cleanup will reliably return the backend to its
original state.
Comment on lines
+95
to
+100
| delta = 0 # intentionally misalign | ||
| current = self.spgr.position.value["goffset"] | ||
| goffset_max = self.spgr.axes["goffset"].range[1] | ||
| direction = 1 if (current + delta < goffset_max) else -1 | ||
|
|
||
| self.spgr.moveRelSync({"goffset": delta * direction}) |
There was a problem hiding this comment.
This test never creates the misalignment it claims to verify.
delta is 0, so Line 100 is a no-op and the assertion can pass even if recovery from an offset regression is broken. Use a non-zero delta that still stays within the axis range.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/test/goffset_test.py` around lines 95 - 100, The test
sets delta = 0 so it never introduces any misalignment; change delta to a
non-zero value that is clamped to stay inside the axis range before calling
self.spgr.moveRelSync. Compute a small positive candidate delta (e.g. 1 step or
a fraction) and then clamp it against goffset_max - current (and a small
epsilon) so it won’t exceed the axis, keep the existing direction calculation
using current, goffset_max and direction, and then call
self.spgr.moveRelSync({"goffset": delta * direction}) to actually create the
misalignment the test intends to verify.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/odemis/acq/align/test/goffset_test.py (2)
44-50:⚠️ Potential issue | 🟠 MajorRe-enable suite cleanup for the spectrograph state.
test_auto_grating_offset()moves the spectrograph, buttearDownClass()is still commented out. That leaves backend state dirty for later tests and keeps this suite order-dependent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 44 - 50, Uncomment and re-enable the class teardown that restores the spectrograph position: restore the `@classmethod` tearDownClass implementation (the one calling cls.spgr.moveAbsSync(cls._original_position)) so test_auto_grating_offset no longer leaves backend state dirty; ensure the method uses the classmethod decorator, wraps the moveAbsSync call in a try/except and logs failures via logging.exception("Failed restoring spectrograph position") so the original position is reliably restored even if errors occur.
95-100:⚠️ Potential issue | 🟠 MajorActually introduce a misalignment before calling the aligner.
deltais0, so Line 100 is a no-op and this test can still pass even if recovery from a goffset regression is broken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 95 - 100, The test currently sets delta = 0 so no misalignment occurs; change delta to a non-zero value (e.g., 1 or a small step) and compute it defensively so it stays within bounds: derive delta from the available headroom (goffset_max - current) or a fixed minimal misalignment, then recompute direction and call self.spgr.moveRelSync({"goffset": delta * direction}) so the test actually introduces a goffset regression before running the aligner; update the delta assignment near the variables current, goffset_max, direction and the subsequent self.spgr.moveRelSync call.
🧹 Nitpick comments (2)
src/odemis/acq/align/test/goffset_test.py (2)
35-55: Add the repo-required annotations and missing hook docstrings.The lifecycle hooks and tests are missing
-> Nonereturn annotations, andsetUpClass()/setUp()also miss function docstrings.Based on learnings: not applicable. As per coding guidelines, "Always use type hints for function parameters and return types in Python code" and "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
Also applies to: 56-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 35 - 55, Add explicit return type annotations (-> None) to the lifecycle methods and tests and add simple reStructuredText-style docstrings for setUpClass, setUp (and the commented tearDownClass if re-enabled) to satisfy repository guidelines; update the signatures for setUpClass and setUp to include "-> None" and add a one- or two-line docstring above each method (e.g., in the methods named setUpClass, setUp, and tearDownClass) describing their purpose without type info.
12-12: Use the publicconcurrent.futures.CancelledErrorimport.Importing from
concurrent.futures._basecouples the test to a private stdlib module. Please switch this tofrom concurrent.futures import CancelledErrorbefore using it in the cancellation assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` at line 12, Replace the private import of CancelledError with the public one: change the import statement that currently reads "from concurrent.futures._base import CancelledError" to "from concurrent.futures import CancelledError" so the test uses the public stdlib symbol CancelledError for cancellation assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 119-120: Add a trailing newline to the end of the file so flake8
W292 is resolved: ensure the file ending after the if __name__ == "__main__": /
unittest.main() block has a single newline character (i.e., terminate the file
with a newline).
- Line 23: The test currently changes the root logger at import via
logging.getLogger().setLevel(logging.DEBUG); instead, stop mutating the root
logger and either create and configure a module logger (logger =
logging.getLogger(__name__); logger.setLevel(logging.DEBUG)) or move the level
change into the specific test using pytest's caplog.set_level(logging.DEBUG) (or
a temporary context) so only that test's logging is affected; update references
to use the module logger if you choose the first approach.
- Around line 84-88: The test is asserting on a tuple because
estimate_goffset_scale(...) returns multiple values; unpack its return into
scale, p0, p1 (e.g., scale, p0, p1 = estimate_goffset_scale(self.spgr,
self.detector)) before running assertions so scale is a float; then keep the
existing assertions self.assertIsInstance(scale, float),
self.assertNotEqual(scale, 0.0), and self.assertTrue(np.isfinite(scale)).
- Around line 111-117: The test never exercises cancellation: after creating the
future with sparc_auto_grating_offset(self.spgr, self.detector) you should
cancel it and assert that calling result() raises a CancelledError instead of
swallowing all exceptions. Replace the try/except block and the lone
self.assertTrue(f.done()) with calling f.cancel(), then use the test helper
(e.g. self.assertRaises(CancelledError, f.result) or a with
self.assertRaises(CancelledError): f.result()) to verify cancellation; ensure
you import the appropriate CancelledError (asyncio.CancelledError or
concurrent.futures.CancelledError) that matches sparc_auto_grating_offset's
future implementation.
---
Duplicate comments:
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 44-50: Uncomment and re-enable the class teardown that restores
the spectrograph position: restore the `@classmethod` tearDownClass implementation
(the one calling cls.spgr.moveAbsSync(cls._original_position)) so
test_auto_grating_offset no longer leaves backend state dirty; ensure the method
uses the classmethod decorator, wraps the moveAbsSync call in a try/except and
logs failures via logging.exception("Failed restoring spectrograph position") so
the original position is reliably restored even if errors occur.
- Around line 95-100: The test currently sets delta = 0 so no misalignment
occurs; change delta to a non-zero value (e.g., 1 or a small step) and compute
it defensively so it stays within bounds: derive delta from the available
headroom (goffset_max - current) or a fixed minimal misalignment, then recompute
direction and call self.spgr.moveRelSync({"goffset": delta * direction}) so the
test actually introduces a goffset regression before running the aligner; update
the delta assignment near the variables current, goffset_max, direction and the
subsequent self.spgr.moveRelSync call.
---
Nitpick comments:
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 35-55: Add explicit return type annotations (-> None) to the
lifecycle methods and tests and add simple reStructuredText-style docstrings for
setUpClass, setUp (and the commented tearDownClass if re-enabled) to satisfy
repository guidelines; update the signatures for setUpClass and setUp to include
"-> None" and add a one- or two-line docstring above each method (e.g., in the
methods named setUpClass, setUp, and tearDownClass) describing their purpose
without type info.
- Line 12: Replace the private import of CancelledError with the public one:
change the import statement that currently reads "from concurrent.futures._base
import CancelledError" to "from concurrent.futures import CancelledError" so the
test uses the public stdlib symbol CancelledError for cancellation assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61440f3e-aaa1-4abe-86dd-08c8df65e054
📒 Files selected for processing (1)
src/odemis/acq/align/test/goffset_test.py
|
|
||
| import odemis | ||
|
|
||
| logging.getLogger().setLevel(logging.DEBUG) |
There was a problem hiding this comment.
Avoid changing the root logger at import time.
This mutates logging for every test loaded after this module and makes suite behavior order-dependent. Prefer a module logger or a temporary level change inside the specific test that needs extra diagnostics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/test/goffset_test.py` at line 23, The test currently
changes the root logger at import via
logging.getLogger().setLevel(logging.DEBUG); instead, stop mutating the root
logger and either create and configure a module logger (logger =
logging.getLogger(__name__); logger.setLevel(logging.DEBUG)) or move the level
change into the specific test using pytest's caplog.set_level(logging.DEBUG) (or
a temporary context) so only that test's logging is affected; update references
to use the module logger if you choose the first approach.
Comment on lines
+84
to
+88
| scale = estimate_goffset_scale(self.spgr, self.detector) | ||
|
|
||
| self.assertIsInstance(scale, float) | ||
| self.assertNotEqual(scale, 0.0) | ||
| self.assertTrue(np.isfinite(scale)) |
There was a problem hiding this comment.
Unpack estimate_goffset_scale() before asserting on scale.
In src/odemis/acq/align/goffset.py, this helper is consumed as scale, p0, p1 = estimate_goffset_scale(...). Here scale is currently the full tuple, so assertIsInstance(scale, float) will fail immediately.
Suggested fix
- scale = estimate_goffset_scale(self.spgr, self.detector)
+ scale, _p0, _p1 = estimate_goffset_scale(self.spgr, self.detector)
self.assertIsInstance(scale, float)
self.assertNotEqual(scale, 0.0)
self.assertTrue(np.isfinite(scale))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/test/goffset_test.py` around lines 84 - 88, The test is
asserting on a tuple because estimate_goffset_scale(...) returns multiple
values; unpack its return into scale, p0, p1 (e.g., scale, p0, p1 =
estimate_goffset_scale(self.spgr, self.detector)) before running assertions so
scale is a float; then keep the existing assertions self.assertIsInstance(scale,
float), self.assertNotEqual(scale, 0.0), and
self.assertTrue(np.isfinite(scale)).
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/odemis/acq/align/goffset.py (1)
409-419:⚠️ Potential issue | 🔴 CriticalThe first-grating alignment is still skipped when
selectoris absent.This loop has the same control-flow bug as the dedicated orchestrator module: the valid single-detector path never calls
sparc_auto_grating_offset()because the work is still nested underif selector:.Possible fix
for d in detectors_sorted: _checkCancelled(future) logging.info("Starting alignment | Detector: %s | Grating: %s", d.name, g0) if selector: selector.moveAbsSync({selector_axes: detector_to_selector[d]}) - future._subfuture = sparc_auto_grating_offset(spectrograph, d) - success = future._subfuture.result() - results[(g0, d.name)] = success - - logging.info("Finished alignment | Detector: %s | Grating: %s | Success: %s", d.name, g0) + future._subfuture = sparc_auto_grating_offset(spectrograph, d) + success = future._subfuture.result() + results[(g0, d.name)] = success + logging.info("Finished alignment | Detector: %s | Grating: %s | Success: %s", d.name, g0, success)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 409 - 419, The loop over detectors_sorted incorrectly nests the alignment call under if selector:, so when selector is None the code never invokes sparc_auto_grating_offset for single-detector runs; move the call to sparc_auto_grating_offset (and the subsequent future._subfuture/result handling, results[(g0, d.name)] = success, and the "Finished alignment" log) out of the if selector: block so that selector.moveAbsSync({selector_axes: detector_to_selector[d]}) only runs when selector is present but sparc_auto_grating_offset(spectrograph, d) always runs for each detector.src/odemis/acq/align/goffset_alignment.py (1)
145-155:⚠️ Potential issue | 🔴 CriticalAlways run the first-grating alignment, even without a selector.
The no-selector path is explicitly allowed on Lines 66-71, but the actual
sparc_auto_grating_offset()call and result write are still nested underif selector:here. In the valid single-detector case, this loop returns no result for the initial grating.Possible fix
for d in detectors_sorted: _checkCancelled(future) logging.info(f"Starting alignment | Detector: {d.name} | Grating: {g0}") if selector: selector.moveAbsSync({selector_axes: detector_to_selector[d]}) - future._subfuture = sparc_auto_grating_offset(spectrograph, d) - success = future._subfuture.result() - results[(g0, d.name)] = success + future._subfuture = sparc_auto_grating_offset(spectrograph, d) + success = future._subfuture.result() + results[(g0, d.name)] = success - logging.info(f"Finished alignment | Detector: {d.name} | Grating: {g0} | Success: {success}") + logging.info(f"Finished alignment | Detector: {d.name} | Grating: {g0} | Success: {success}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset_alignment.py` around lines 145 - 155, The loop currently only calls sparc_auto_grating_offset and writes to results[(g0, d.name)] when selector is present, so the first-grating alignment is skipped when selector is None; update the loop over detectors_sorted to always invoke sparc_auto_grating_offset (and await its result via future._subfuture.result()) and assign results[(g0, d.name)] = success regardless of selector presence—keep the selector.moveAbsSync({selector_axes: detector_to_selector[d]}) inside the if selector: block but move the sparc_auto_grating_offset(spectrograph, d) call, future._subfuture assignment, result collection, and the "Finished alignment" log out of that if so they run for both selector and no-selector paths.
🧹 Nitpick comments (1)
src/odemis/acq/align/goffset.py (1)
17-27: Add annotations to the new public helper.
gaussian()is part of the new API surface, but its parameters/return type are untyped and the docstring fields are not valid reST (:param x0 = ...). Please make this helper fully typed/documented like the rest of the module.Possible fix
-def gaussian(x, amplitude, x0, width): +def gaussian( + x: numpy.ndarray, + amplitude: float, + x0: float, + width: float, +) -> numpy.ndarray: """ Gaussian function (for curve fitting). :param x: input coordinates - :param amplitude: peak intensity - :param x0 = peak's center - :param width = standard deviation + :param amplitude: peak intensity + :param x0: peak center + :param width: standard deviation :return: Gaussian function evaluated at x """As per coding guidelines,
**/*.py: Always use type hints for function parameters and return types in Python codeandInclude docstrings for all functions and classes, following the reStructuredText style guide (without type information)`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 17 - 27, The gaussian helper lacks type hints, a proper reST docstring and a return statement; update def gaussian to include type hints (e.g. x: numpy.ndarray | float, amplitude: float, x0: float, width: float) with a return type of numpy.ndarray | float (or use numpy.typing.NDArray if you prefer arrays), add a reStructuredText-style docstring using :param x:, :param amplitude:, :param x0:, :param width: and :return:, and ensure the function returns the computed intensity (i.e., add a return intensity at the end) so gaussian is fully typed and documented consistent with the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/align/goffset_alignment.py`:
- Around line 17-25: The _checkCancelled function is inspecting the wrong
attributes (_function_lock/_function_state) and should use the same cancellation
fields initialized by auto_align_grating_detector_offsets (f._task_lock and
f._task_state); update _checkCancelled to acquire future._task_lock and test
future._task_state == CANCELLED, then raise CancelledError as before, ensuring
you reference the CANCELLED symbol and CancelledError so behavior remains
identical.
- Line 48: The undefined forward-reference to Stream in type annotations should
be resolved by importing Stream under a TYPE_CHECKING guard: add "from typing
import TYPE_CHECKING" and inside "if TYPE_CHECKING:" import "Stream" from
"odemis.acq.stream" so annotations like Optional[List['Stream']] (seen in the
function/method signatures in goffset_alignment.py) are recognized by linters
without importing Stream at runtime; alternatively, you can directly import
Stream at top-level if you prefer.
In `@src/odemis/acq/align/goffset.py`:
- Around line 228-229: The loop calling _checkCancelled() is missing the
required future argument; update the for-loop at "for i in range(max_it):" to
call _checkCancelled(future) (or the local future variable in scope) so the
helper receives the current Future object; ensure the same future variable used
elsewhere in this alignment routine is passed into _checkCancelled to avoid the
TypeError before the alignment loop begins.
- Around line 271-278: The canceller _cancel_sparc_auto_grating_offset currently
flips future._task_state to CANCELLED but returns None so
model.ProgressiveFuture.cancel() doesn't detect success; update
_cancel_sparc_auto_grating_offset (accepting future: model.ProgressiveFuture) to
return a truthy value (e.g., True) after acquiring future._task_lock and setting
future._task_state = CANCELLED, ensuring the outer cancel() call observes the
task was cancelled; preserve the lock usage and existing CANCELLED constant and
do not change other semantics.
- Around line 128-134: The temporary move via spgr.moveRelSync({"goffset":
actual_delta}) must always be undone even if detector.data.get() or
find_peak_position() raise; wrap the measurement and fitting steps in a
try/finally around the call to spgr.moveRelSync so that
spgr.moveRelSync({"goffset": -actual_delta}) runs in the finally block, and
re-raise the exception after rollback so callers still see the error. Ensure the
finally references the same actual_delta and use the existing
functions/spgr.moveRelSync, detector.data.get, and find_peak_position to locate
where to add the try/finally.
- Around line 253-256: The syntax error is caused by having logging.debug(...)
and spgr.moveRelSync(...) on the same line; edit the code in goffset.py (around
the loop in the function handling alignment) to place
spgr.moveRelSync({"goffset": delta_goffset}) on its own line after the
logging.debug call with correct indentation so the two statements are separate
(locate the logging.debug(...) and split it so spgr.moveRelSync is a standalone
statement).
---
Duplicate comments:
In `@src/odemis/acq/align/goffset_alignment.py`:
- Around line 145-155: The loop currently only calls sparc_auto_grating_offset
and writes to results[(g0, d.name)] when selector is present, so the
first-grating alignment is skipped when selector is None; update the loop over
detectors_sorted to always invoke sparc_auto_grating_offset (and await its
result via future._subfuture.result()) and assign results[(g0, d.name)] =
success regardless of selector presence—keep the
selector.moveAbsSync({selector_axes: detector_to_selector[d]}) inside the if
selector: block but move the sparc_auto_grating_offset(spectrograph, d) call,
future._subfuture assignment, result collection, and the "Finished alignment"
log out of that if so they run for both selector and no-selector paths.
In `@src/odemis/acq/align/goffset.py`:
- Around line 409-419: The loop over detectors_sorted incorrectly nests the
alignment call under if selector:, so when selector is None the code never
invokes sparc_auto_grating_offset for single-detector runs; move the call to
sparc_auto_grating_offset (and the subsequent future._subfuture/result handling,
results[(g0, d.name)] = success, and the "Finished alignment" log) out of the if
selector: block so that selector.moveAbsSync({selector_axes:
detector_to_selector[d]}) only runs when selector is present but
sparc_auto_grating_offset(spectrograph, d) always runs for each detector.
---
Nitpick comments:
In `@src/odemis/acq/align/goffset.py`:
- Around line 17-27: The gaussian helper lacks type hints, a proper reST
docstring and a return statement; update def gaussian to include type hints
(e.g. x: numpy.ndarray | float, amplitude: float, x0: float, width: float) with
a return type of numpy.ndarray | float (or use numpy.typing.NDArray if you
prefer arrays), add a reStructuredText-style docstring using :param x:, :param
amplitude:, :param x0:, :param width: and :return:, and ensure the function
returns the computed intensity (i.e., add a return intensity at the end) so
gaussian is fully typed and documented consistent with the module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 063ae5ca-68f1-4dc3-805c-dfd4c7f560d0
📒 Files selected for processing (2)
src/odemis/acq/align/goffset.pysrc/odemis/acq/align/goffset_alignment.py
Comment on lines
+17
to
+25
| def _checkCancelled(future: "model.ProgressiveFuture"): | ||
|
|
||
| """ | ||
| Check if the future has been cancelled, and if so raise CancelledError. | ||
| """ | ||
|
|
||
| with future._function_lock: | ||
| if future._function_state == CANCELLED: | ||
| raise CancelledError() |
There was a problem hiding this comment.
Use the same cancellation fields that the future actually initializes.
auto_align_grating_detector_offsets() sets f._task_lock / f._task_state on Lines 83-84, but _checkCancelled() reads _function_lock / _function_state. The first cancellation check in the worker will fail with AttributeError instead of stopping cleanly.
Possible fix
-def _checkCancelled(future: "model.ProgressiveFuture"):
+def _checkCancelled(future: "model.ProgressiveFuture") -> None:
@@
- with future._function_lock:
- if future._function_state == CANCELLED:
+ with future._task_lock:
+ if future._task_state == CANCELLED:
raise CancelledError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/goffset_alignment.py` around lines 17 - 25, The
_checkCancelled function is inspecting the wrong attributes
(_function_lock/_function_state) and should use the same cancellation fields
initialized by auto_align_grating_detector_offsets (f._task_lock and
f._task_state); update _checkCancelled to acquire future._task_lock and test
future._task_state == CANCELLED, then raise CancelledError as before, ensuring
you reference the CANCELLED symbol and CancelledError so behavior remains
identical.
Comment on lines
+128
to
+134
| # move and measure | ||
| spgr.moveRelSync({"goffset": actual_delta}) | ||
| data1 = detector.data.get(asap=False) | ||
| p1 = find_peak_position(data1) | ||
|
|
||
| # return back to start | ||
| spgr.moveRelSync({"goffset": -actual_delta}) |
There was a problem hiding this comment.
Always restore goffset, even when acquisition or fitting fails.
If detector.data.get() or find_peak_position() raises after the temporary move, the rollback on Line 134 never executes and the spectrograph stays at the test offset. That leaves the hardware in a mutated state for the next calibration step.
Possible fix
# move and measure
spgr.moveRelSync({"goffset": actual_delta})
- data1 = detector.data.get(asap=False)
- p1 = find_peak_position(data1)
-
- # return back to start
- spgr.moveRelSync({"goffset": -actual_delta})
+ try:
+ data1 = detector.data.get(asap=False)
+ p1 = find_peak_position(data1)
+ finally:
+ # return back to start
+ spgr.moveRelSync({"goffset": -actual_delta})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/goffset.py` around lines 128 - 134, The temporary move
via spgr.moveRelSync({"goffset": actual_delta}) must always be undone even if
detector.data.get() or find_peak_position() raise; wrap the measurement and
fitting steps in a try/finally around the call to spgr.moveRelSync so that
spgr.moveRelSync({"goffset": -actual_delta}) runs in the finally block, and
re-raise the exception after rollback so callers still see the error. Ensure the
finally references the same actual_delta and use the existing
functions/spgr.moveRelSync, detector.data.get, and find_peak_position to locate
where to add the try/finally.
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
from
2c1fdcf to
5eccdad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
src/odemis/acq/align/test/goffset_test.py (5)
43-49:⚠️ Potential issue | 🟠 MajorRestore the spectrograph state after this suite.
The tearDownClass is commented out, leaving the spectrograph in a modified state after tests complete. This creates order-dependent test behavior. Please uncomment and enable the cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 43 - 49, Uncomment and re-enable the class-level teardown to restore the spectrograph state: reinstate the `@classmethod` tearDownClass in the test class and ensure it calls cls.spgr.moveAbsSync(cls._original_position) inside a try/except that uses logging.exception("Failed restoring spectrograph position") on failure; make sure cls._original_position is set in setUpClass and that cls.spgr is available so the spectrograph is reliably returned to its original position after the test suite.
83-87:⚠️ Potential issue | 🔴 CriticalUnpack
estimate_goffset_scale()return value before asserting.Per the implementation in
goffset.py,estimate_goffset_scale()returns a tuple(scale, p0, p1). The current code assigns the full tuple toscale, causingassertIsInstance(scale, float)to fail.Suggested fix
- scale = estimate_goffset_scale(self.spgr, self.detector) + scale, _p0, _p1 = estimate_goffset_scale(self.spgr, self.detector) self.assertIsInstance(scale, float)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 83 - 87, The test is assigning the entire tuple returned by estimate_goffset_scale() to scale; update the test to unpack the tuple returned by estimate_goffset_scale(self.spgr, self.detector) into (scale, p0, p1) (or similar names) and then run the existing assertions on scale (assertIsInstance(scale, float), assertNotEqual(scale, 0.0), assertTrue(np.isfinite(scale))); reference estimate_goffset_scale to locate the implementation and adjust the test accordingly.
117-119:⚠️ Potential issue | 🟡 MinorFix whitespace and trailing newline issues.
Pipeline failures indicate:
- Line 117: blank line contains whitespace (W293)
- Line 207: no newline at end of file (W292)
Run
autopep8 --in-place --select W291,W292,W293,W391to fix these. As per coding guidelines, clean up code using autopep8.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 117 - 119, The file src/odemis/acq/align/test/goffset_test.py has trailing whitespace on a blank line (W293 at line 117) and is missing a newline at EOF (W292 at line 207); run autopep8 with the suggested flags (autopep8 --in-place --select W291,W292,W293,W391) on goffset_test.py or manually remove trailing spaces and add a final newline so the file ends with a single newline, then commit the cleaned file.
110-116:⚠️ Potential issue | 🟠 MajorTest never exercises cancellation.
The test waits for
result(), catches all exceptions silently, then only checksdone(). This passes on normal completion or any failure. To properly test cancellation, callf.cancel()and assertCancelledErroris raised.Suggested fix
f = sparc_auto_grating_offset(self.spgr, self.detector) - # Wait for the result or a timeout - try: - f.result(timeout=5) - except: - pass - self.assertTrue(f.done()) + time.sleep(0.5) # Let task start + cancelled = f.cancel() + self.assertTrue(cancelled) + with self.assertRaises(CancelledError): + f.result(timeout=5) + self.assertTrue(f.cancelled())Also, the bare
except:on Line 114 violates E722 and S110.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 110 - 116, Modify the test around sparc_auto_grating_offset/self.spgr to actually exercise cancellation: call f.cancel() after starting the future, then assert that awaiting f.result() raises the appropriate CancelledError (use asyncio.CancelledError or concurrent.futures.CancelledError consistent with how sparc_auto_grating_offset returns a future), and replace the bare except: with a specific exception assertion (e.g., with self.assertRaises(CancelledError): f.result(timeout=5)) or a try/except that only catches CancelledError; ensure you assert f.cancelled() or that the CancelledError was raised rather than silently swallowing all exceptions.
94-99:⚠️ Potential issue | 🟠 MajorTest never introduces misalignment due to
delta = 0.With
delta = 0, themoveRelSynccall on Line 99 is a no-op. The test claims to verify recovery from an intentional misalignment but never creates one. Use a non-zero delta.Suggested fix
- delta = 0 # intentionally misalign + delta = 100 # intentionally misalign by a small amount current = self.spgr.position.value["goffset"] goffset_max = self.spgr.axes["goffset"].range[1] + goffset_min = self.spgr.axes["goffset"].range[0] - direction = 1 if (current + delta < goffset_max) else -1 + # Ensure delta stays within axis bounds + if current + delta > goffset_max: + direction = -1 + elif current - delta < goffset_min: + direction = 1 + else: + direction = 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 94 - 99, The test sets delta = 0 so no misalignment is introduced; change delta to a non-zero value (e.g., a small safe offset) so the call to self.spgr.moveRelSync({"goffset": delta * direction}) actually moves the axis; ensure the chosen delta stays within bounds by computing direction using current = self.spgr.position.value["goffset"] and goffset_max = self.spgr.axes["goffset"].range[1] (as already used) so the relative move is valid and triggers the intended misalignment/recovery behavior.src/odemis/acq/align/goffset.py (5)
61-71:⚠️ Potential issue | 🟠 MajorUse clipped
weightsin the centroid calculation.The code computes
weights = window_data.clip(min=0)but then useswindow_datain the weighted average calculation. If negative values exist inwindow_data, they will distort the centroid estimate.Suggested fix
else: - weighted_avg = float(numpy.sum(window_idx * window_data) / numpy.sum(window_data)) + weighted_avg = float(numpy.sum(window_idx * weights) / numpy.sum(weights))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 61 - 71, The centroid uses raw window_data instead of the non-negative weights previously computed; change the weighted average calculation in the else branch to use weights (the clipped values) and weights.sum() instead of window_data and numpy.sum(window_data) so negative values don't skew the estimate—update the assignment to weighted_avg to compute float(numpy.sum(window_idx * weights) / numpy.sum(weights)) while keeping the existing peak_idx fallback and logging intact.
220-220:⚠️ Potential issue | 🟠 MajorUse zero-based midpoint for
center_target.
find_peak_position()returns zero-based pixel coordinates. For an even-width detector, the center should be(width - 1) / 2, notwidth / 2. With the current formula, a perfectly centered peak reads as 0.5 px off.Suggested fix
- center_target = detector.resolution.value[0] / 2 # adjust if 0 is not the center + center_target = (detector.resolution.value[0] - 1) / 2 # zero-based midpoint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` at line 220, The center_target calculation uses detector.resolution.value[0] / 2 which assumes one-based centering and yields a 0.5 px bias for even widths; change it to use a zero-based midpoint: set center_target = (detector.resolution.value[0] - 1) / 2 so it matches find_peak_position()’s zero-based pixel coordinates and update any accompanying comment accordingly.
253-256:⚠️ Potential issue | 🔴 CriticalCritical syntax error: two statements on one line.
The pipeline fails because
logging.debug(...)andspgr.moveRelSync(...)are on the same line without a separator. This prevents the module from being imported.Suggested fix
logging.debug( "DEBUG | Iter: %d | Peak: %.1f | Error: %.1f | Move: %.4f | Total Change: %.4f", i, peak_px, error_px, delta_goffset, total_goffset_displacement - ) spgr.moveRelSync({"goffset": delta_goffset}) + ) + spgr.moveRelSync({"goffset": delta_goffset})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 253 - 256, The logging.debug(...) call and spgr.moveRelSync({"goffset": delta_goffset}) are accidentally on the same line causing a syntax error; update the block (around the loop that logs iteration status) to place logging.debug(...) and spgr.moveRelSync(...) on separate statements/lines so each is its own statement (preserve the same arguments and indentation), ensuring the logging.debug(...) call is followed by a newline before calling spgr.moveRelSync.
271-278:⚠️ Potential issue | 🟠 MajorReturn a boolean from the task canceller.
model.CancellableFuture.cancel()only marks the future as cancelled when the canceller returns a truthy value. Currently,_cancel_sparc_auto_grating_offsetreturnsNone, so cancellation may appear to fail even though the state was updated.Suggested fix
-def _cancel_sparc_auto_grating_offset(future: model.ProgressiveFuture): - +def _cancel_sparc_auto_grating_offset(future: model.ProgressiveFuture) -> bool: """ Canceller of _do_sparc_auto_grating_offset task. """ with future._task_lock: + if future._task_state == FINISHED: + return False future._task_state = CANCELLED + return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 271 - 278, The canceller _cancel_sparc_auto_grating_offset currently sets future._task_state = CANCELLED inside the with future._task_lock block but returns None; change it so after acquiring the lock and setting the state it returns True (a truthy value) so model.CancellableFuture.cancel() will treat the cancel as successful—i.e., inside _cancel_sparc_auto_grating_offset keep the with future._task_lock, set future._task_state = CANCELLED, then return True.
128-134:⚠️ Potential issue | 🟠 MajorWrap measurement in
try/finallyto ensure goffset is restored.If
detector.data.get()orfind_peak_position()raises after the spectrograph move, the rollback on Line 134 never executes, leaving the hardware in a modified state.Suggested fix
# move and measure spgr.moveRelSync({"goffset": actual_delta}) - data1 = detector.data.get(asap=False) - p1 = find_peak_position(data1) - - # return back to start - spgr.moveRelSync({"goffset": -actual_delta}) + try: + data1 = detector.data.get(asap=False) + p1 = find_peak_position(data1) + finally: + # return back to start + spgr.moveRelSync({"goffset": -actual_delta})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 128 - 134, After calling spgr.moveRelSync({"goffset": actual_delta}) you must wrap the measurement block in a try/finally so that spgr.moveRelSync({"goffset": -actual_delta}) always runs even if detector.data.get(...) or find_peak_position(...) raises; modify the block around spgr.moveRelSync, detector.data.get, and find_peak_position to perform the measurement in the try and put the rollback moveRelSync({"goffset": -actual_delta}) in the finally, preserving the current actual_delta variable and re-raising any exception after cleanup.
🧹 Nitpick comments (5)
src/odemis/acq/align/goffset.py (3)
92-92: Add type hint fordeltaparameter.The
deltaparameter lacks a type hint.Suggested fix
-def estimate_goffset_scale(spgr: model.Actuator, detector: model.Detector, delta=5.0, retries = 1) -> Tuple[float, float, float]: +def estimate_goffset_scale(spgr: model.Actuator, detector: model.Detector, delta: float = 5.0, retries: int = 1) -> Tuple[float, float, float]:Also note the extra space before
= 1which should be removed per PEP8.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` at line 92, The function signature for estimate_goffset_scale is missing a type hint for the delta parameter and has an extra space before the default for retries; update the signature in estimate_goffset_scale(spgr: model.Actuator, detector: model.Detector, delta=5.0, retries = 1) to add an appropriate type (e.g., delta: float) and remove the space before the equals in retries (retries=1) so it reads something like delta: float = 5.0 and retries=1.
386-386: Avoid f-strings in logging calls.Using f-strings in logging statements evaluates the string even when the log level is disabled. Use
%-style formatting instead.Suggested fix
- logging.info(f"Available gratings: {list(spectrograph.axes['grating'].choices.keys())}") + logging.info("Available gratings: %s", list(spectrograph.axes['grating'].choices.keys()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` at line 386, The logging call uses an f-string which evaluates eagerly; change the logging.info call that references spectrograph.axes['grating'].choices.keys() to use %-style formatting so the message is formatted lazily (e.g. logging.info("Available gratings: %s", list(spectrograph.axes['grating'].choices.keys()))). Update the logging invocation where the current f-string appears (the logging.info line in goffset.py) to this %-style form.
17-28: Add type hints to thegaussianfunction parameters.As per coding guidelines, always use type hints for function parameters and return types.
Suggested fix
-def gaussian(x, amplitude, x0, width): +def gaussian(x: numpy.ndarray, amplitude: float, x0: float, width: float) -> numpy.ndarray:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 17 - 28, The gaussian function lacks type hints; update the signature of gaussian to annotate parameters (x, amplitude, x0, width) and its return type — e.g., use numpy.typing.ArrayLike or numpy.ndarray/float for x and float for amplitude, x0 and width, and annotate the return as numpy.ndarray (or Union[float, numpy.ndarray] if you accept scalar input); add any necessary imports from typing or numpy.typing and ensure the docstring remains accurate.src/odemis/acq/align/test/goffset_test.py (2)
186-190: Avoidprint()in tests; useloggingor test assertions.Using
print()for diagnostic output is discouraged. Either uselogging.warning()or convert this into a proper test assertion that fails if the data is unexpectedly flat.Suggested fix
# check data is not flat if data.max() == data.min(): - print("WARNING: sp-ccd is returning a flat image!") + logging.warning("sp-ccd is returning a flat image!") else: - print(f"sp-ccd signal range: {data.min()} to {data.max()}") + logging.debug("sp-ccd signal range: %s to %s", data.min(), data.max())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 186 - 190, Replace the ad-hoc print-based diagnostics in the test by either using the logging module or a proper test assertion: remove the print statements that reference the test variable `data` and instead import Python's logging and call logging.warning(...) when you want non-failing diagnostic output, or better, replace the branch with an assertion such as `assert data.max() != data.min(), "sp-ccd returned a flat image"` so the test fails on unexpected flat data; update the test function containing `data` accordingly and add `import logging` at the top if you choose logging.
144-144: Prefernext(iter(...))over list slicing for single element.Static analysis suggests using
next(iter(self.spgr.axes["grating"].choices.keys()))instead of converting to a list and indexing[0].Suggested fix
- first_grating = list(self.spgr.axes["grating"].choices.keys())[0] + first_grating = next(iter(self.spgr.axes["grating"].choices.keys()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` at line 144, Replace the list conversion and index access when grabbing the first grating with an iterator-based approach: locate the assignment to first_grating in goffset_test.py (the line using list(self.spgr.axes["grating"].choices.keys())[0]) and change it to use next(iter(...)) over the same keys expression (self.spgr.axes["grating"].choices.keys()) to avoid building an intermediate list and make intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/align/goffset.py`:
- Around line 413-418: The loop currently only sets results[(g0, d.name)] inside
the if selector: branch, so when selector is None the results entry is never
populated; modify the loop around detector variable d to handle the else case by
invoking sparc_auto_grating_offset for that detector (as done when selector is
present), obtaining the success value (via the returned future or direct call)
and assigning results[(g0, d.name)] = success; ensure you reference and set
future._subfuture or otherwise capture the return of
sparc_auto_grating_offset(spectrograph, d) and store its result for consistency
with the selector path.
---
Duplicate comments:
In `@src/odemis/acq/align/goffset.py`:
- Around line 61-71: The centroid uses raw window_data instead of the
non-negative weights previously computed; change the weighted average
calculation in the else branch to use weights (the clipped values) and
weights.sum() instead of window_data and numpy.sum(window_data) so negative
values don't skew the estimate—update the assignment to weighted_avg to compute
float(numpy.sum(window_idx * weights) / numpy.sum(weights)) while keeping the
existing peak_idx fallback and logging intact.
- Line 220: The center_target calculation uses detector.resolution.value[0] / 2
which assumes one-based centering and yields a 0.5 px bias for even widths;
change it to use a zero-based midpoint: set center_target =
(detector.resolution.value[0] - 1) / 2 so it matches find_peak_position()’s
zero-based pixel coordinates and update any accompanying comment accordingly.
- Around line 253-256: The logging.debug(...) call and
spgr.moveRelSync({"goffset": delta_goffset}) are accidentally on the same line
causing a syntax error; update the block (around the loop that logs iteration
status) to place logging.debug(...) and spgr.moveRelSync(...) on separate
statements/lines so each is its own statement (preserve the same arguments and
indentation), ensuring the logging.debug(...) call is followed by a newline
before calling spgr.moveRelSync.
- Around line 271-278: The canceller _cancel_sparc_auto_grating_offset currently
sets future._task_state = CANCELLED inside the with future._task_lock block but
returns None; change it so after acquiring the lock and setting the state it
returns True (a truthy value) so model.CancellableFuture.cancel() will treat the
cancel as successful—i.e., inside _cancel_sparc_auto_grating_offset keep the
with future._task_lock, set future._task_state = CANCELLED, then return True.
- Around line 128-134: After calling spgr.moveRelSync({"goffset": actual_delta})
you must wrap the measurement block in a try/finally so that
spgr.moveRelSync({"goffset": -actual_delta}) always runs even if
detector.data.get(...) or find_peak_position(...) raises; modify the block
around spgr.moveRelSync, detector.data.get, and find_peak_position to perform
the measurement in the try and put the rollback moveRelSync({"goffset":
-actual_delta}) in the finally, preserving the current actual_delta variable and
re-raising any exception after cleanup.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 43-49: Uncomment and re-enable the class-level teardown to restore
the spectrograph state: reinstate the `@classmethod` tearDownClass in the test
class and ensure it calls cls.spgr.moveAbsSync(cls._original_position) inside a
try/except that uses logging.exception("Failed restoring spectrograph position")
on failure; make sure cls._original_position is set in setUpClass and that
cls.spgr is available so the spectrograph is reliably returned to its original
position after the test suite.
- Around line 83-87: The test is assigning the entire tuple returned by
estimate_goffset_scale() to scale; update the test to unpack the tuple returned
by estimate_goffset_scale(self.spgr, self.detector) into (scale, p0, p1) (or
similar names) and then run the existing assertions on scale
(assertIsInstance(scale, float), assertNotEqual(scale, 0.0),
assertTrue(np.isfinite(scale))); reference estimate_goffset_scale to locate the
implementation and adjust the test accordingly.
- Around line 117-119: The file src/odemis/acq/align/test/goffset_test.py has
trailing whitespace on a blank line (W293 at line 117) and is missing a newline
at EOF (W292 at line 207); run autopep8 with the suggested flags (autopep8
--in-place --select W291,W292,W293,W391) on goffset_test.py or manually remove
trailing spaces and add a final newline so the file ends with a single newline,
then commit the cleaned file.
- Around line 110-116: Modify the test around
sparc_auto_grating_offset/self.spgr to actually exercise cancellation: call
f.cancel() after starting the future, then assert that awaiting f.result()
raises the appropriate CancelledError (use asyncio.CancelledError or
concurrent.futures.CancelledError consistent with how sparc_auto_grating_offset
returns a future), and replace the bare except: with a specific exception
assertion (e.g., with self.assertRaises(CancelledError): f.result(timeout=5)) or
a try/except that only catches CancelledError; ensure you assert f.cancelled()
or that the CancelledError was raised rather than silently swallowing all
exceptions.
- Around line 94-99: The test sets delta = 0 so no misalignment is introduced;
change delta to a non-zero value (e.g., a small safe offset) so the call to
self.spgr.moveRelSync({"goffset": delta * direction}) actually moves the axis;
ensure the chosen delta stays within bounds by computing direction using current
= self.spgr.position.value["goffset"] and goffset_max =
self.spgr.axes["goffset"].range[1] (as already used) so the relative move is
valid and triggers the intended misalignment/recovery behavior.
---
Nitpick comments:
In `@src/odemis/acq/align/goffset.py`:
- Line 92: The function signature for estimate_goffset_scale is missing a type
hint for the delta parameter and has an extra space before the default for
retries; update the signature in estimate_goffset_scale(spgr: model.Actuator,
detector: model.Detector, delta=5.0, retries = 1) to add an appropriate type
(e.g., delta: float) and remove the space before the equals in retries
(retries=1) so it reads something like delta: float = 5.0 and retries=1.
- Line 386: The logging call uses an f-string which evaluates eagerly; change
the logging.info call that references
spectrograph.axes['grating'].choices.keys() to use %-style formatting so the
message is formatted lazily (e.g. logging.info("Available gratings: %s",
list(spectrograph.axes['grating'].choices.keys()))). Update the logging
invocation where the current f-string appears (the logging.info line in
goffset.py) to this %-style form.
- Around line 17-28: The gaussian function lacks type hints; update the
signature of gaussian to annotate parameters (x, amplitude, x0, width) and its
return type — e.g., use numpy.typing.ArrayLike or numpy.ndarray/float for x and
float for amplitude, x0 and width, and annotate the return as numpy.ndarray (or
Union[float, numpy.ndarray] if you accept scalar input); add any necessary
imports from typing or numpy.typing and ensure the docstring remains accurate.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 186-190: Replace the ad-hoc print-based diagnostics in the test by
either using the logging module or a proper test assertion: remove the print
statements that reference the test variable `data` and instead import Python's
logging and call logging.warning(...) when you want non-failing diagnostic
output, or better, replace the branch with an assertion such as `assert
data.max() != data.min(), "sp-ccd returned a flat image"` so the test fails on
unexpected flat data; update the test function containing `data` accordingly and
add `import logging` at the top if you choose logging.
- Line 144: Replace the list conversion and index access when grabbing the first
grating with an iterator-based approach: locate the assignment to first_grating
in goffset_test.py (the line using
list(self.spgr.axes["grating"].choices.keys())[0]) and change it to use
next(iter(...)) over the same keys expression
(self.spgr.axes["grating"].choices.keys()) to avoid building an intermediate
list and make intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b56e39ce-e95a-464d-aac1-f7d76f609ec0
📒 Files selected for processing (2)
src/odemis/acq/align/goffset.pysrc/odemis/acq/align/test/goffset_test.py
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
6 times, most recently
from
b8111d3 to
de03ff2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (14)
src/odemis/acq/align/test/goffset_test.py (5)
43-49:⚠️ Potential issue | 🟠 MajorRestore the spectrograph state in
tearDownClass().Leaving the cleanup path commented out makes later tests order-dependent and can leave the backend in a modified state after this suite finishes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 43 - 49, Uncomment and re-enable the tearDownClass cleanup so the spectrograph state is restored: restore the commented-out `@classmethod` tearDownClass in the test class, call cls.spgr.moveAbsSync(cls._original_position) inside a try/except, and on exception call logging.exception("Failed restoring spectrograph position") so that the spectrograph (cls.spgr) is returned to cls._original_position after the suite runs.
83-87:⚠️ Potential issue | 🔴 CriticalUnpack
estimate_goffset_scale()before asserting onscale.The helper returns
(scale, p0, p1), soscaleis currently a tuple andassertIsInstance(scale, float)will fail immediately.Suggested fix
- scale = estimate_goffset_scale(self.spgr, self.detector) + scale, _p0, _p1 = estimate_goffset_scale(self.spgr, self.detector)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 83 - 87, The test is calling estimate_goffset_scale(self.spgr, self.detector) but that function returns a tuple (scale, p0, p1); update the test in goffset_test.py to unpack the returned tuple (e.g. scale, p0, p1 = estimate_goffset_scale(...)) and then run the existing assertions against scale (assertIsInstance(scale, float), assertNotEqual(scale, 0.0), assertTrue(np.isfinite(scale))). Ensure you reference estimate_goffset_scale and the local variables p0/p1 when making the change.
94-99:⚠️ Potential issue | 🟠 MajorUse a non-zero delta so this test actually creates misalignment.
delta = 0turns the relative move into a no-op, so this can pass even if recovery from an offset regression is broken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 94 - 99, The test sets delta = 0 so the relative move is a no-op; change the test in goffset_test.py to use a non-zero delta (e.g., a small amount appropriate for the system) instead of 0, keeping the existing computation of current = self.spgr.position.value["goffset"], goffset_max = self.spgr.axes["goffset"].range[1], direction = 1 if (current + delta < goffset_max) else -1 and then calling self.spgr.moveRelSync({"goffset": delta * direction}) so the test actually introduces a misalignment to verify recovery.
110-116:⚠️ Potential issue | 🟠 MajorExercise cancellation instead of swallowing every exception.
This path never calls
cancel(), so it can pass on normal completion, timeout, or unrelated failures.Suggested fix
f = sparc_auto_grating_offset(self.spgr, self.detector) - # Wait for the result or a timeout - try: - f.result(timeout=5) - except: - pass + self.assertTrue(f.cancel()) + with self.assertRaises(CancelledError): + f.result(timeout=5) self.assertTrue(f.done())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 110 - 116, The test currently swallows all exceptions when waiting on the future returned by sparc_auto_grating_offset(self.spgr, self.detector); change the try/except so you only catch the timeout, call f.cancel() when a timeout occurs, and let other exceptions propagate. Specifically, replace the broad except with catching concurrent.futures.TimeoutError (or asyncio.TimeoutError if using asyncio futures), call f.cancel() inside that except block, and avoid catching Exception or bare except so real failures still surface; keep the final assertion self.assertTrue(f.done()) to verify the future was finished or cancelled.
22-22:⚠️ Potential issue | 🟡 MinorStop changing the root logger at import time.
This leaks
DEBUGlogging into unrelated tests and makes suite behavior order-dependent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` at line 22, The module is calling logging.getLogger().setLevel(logging.DEBUG) at import time which mutates the root logger and leaks DEBUG into other tests; remove that import-time root-level change and instead obtain a module logger via logger = logging.getLogger(__name__) and use that logger for local debug calls, or move any test-specific logging configuration into the test setup/teardown (or an if __name__ == '__main__' guard) so you do not set the root logger level during import.src/odemis/acq/align/goffset.py (5)
127-133:⚠️ Potential issue | 🟠 MajorAlways undo the probe move in a
finallyblock.If acquisition or fitting fails after the temporary
goffsetmove, the rollback never runs and the spectrograph stays at the test offset.Suggested fix
# move and measure spgr.moveRelSync({"goffset": actual_delta}) - data1 = detector.data.get(asap=False) - p1 = find_peak_position(data1) - - # return back to start - spgr.moveRelSync({"goffset": -actual_delta}) + try: + data1 = detector.data.get(asap=False) + p1 = find_peak_position(data1) + finally: + # return back to start + spgr.moveRelSync({"goffset": -actual_delta})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 127 - 133, The temporary spectrograph move using spgr.moveRelSync({"goffset": actual_delta}) must be reverted inside a finally block so the rollback always runs even if detector.data.get or find_peak_position raise; wrap the "move and measure" sequence (spgr.moveRelSync({"goffset": actual_delta}), data1 = detector.data.get(asap=False), p1 = find_peak_position(data1)) in try and perform spgr.moveRelSync({"goffset": -actual_delta}) in a finally clause, preserving any raised exceptions (re-raise if needed) so the spectrograph is always returned to the start position.
271-278:⚠️ Potential issue | 🟠 MajorReturn a boolean from the task canceller.
ProgressiveFuture.cancel()depends on the task canceller returning truthy. This helper currently returnsNone, so callers can seecancel()fail even though_task_statewas flipped locally.Suggested fix
-def _cancel_sparc_auto_grating_offset(future: model.ProgressiveFuture): +def _cancel_sparc_auto_grating_offset(future: model.ProgressiveFuture) -> bool: @@ """ Canceller of _do_sparc_auto_grating_offset task. """ with future._task_lock: - future._task_state = CANCELLED + if future._task_state == FINISHED: + return False + future._task_state = CANCELLED + return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 271 - 278, The canceller _cancel_sparc_auto_grating_offset currently sets future._task_state = CANCELLED under future._task_lock but returns None, which makes ProgressiveFuture.cancel() see a falsy result; update _cancel_sparc_auto_grating_offset to return a truthy value (e.g., True) after successfully acquiring the lock and setting future._task_state, ensuring ProgressiveFuture.cancel() observes a successful cancellation; keep the lock usage and state assignment as-is and simply add the boolean return at the end.
64-72:⚠️ Potential issue | 🟠 MajorUse the clipped positive weights in the centroid fallback.
The guard checks
weights.sum(), but the centroid still useswindow_data. Negative baseline values can cancel the denominator or pull the estimate away from the positive lobe.Suggested fix
- weighted_avg = float(numpy.sum(window_idx * window_data) / numpy.sum(window_data)) + weighted_avg = float(numpy.sum(window_idx * weights) / numpy.sum(weights))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 64 - 72, The centroid fallback is inconsistent: the guard checks weights.sum() but the centroid uses window_data, allowing negative baseline values to skew the result; change the centroid computation for weighted_avg to use the clipped positive weights (use numpy.sum(window_idx * weights) / numpy.sum(weights)) so the numerator and denominator reference the same positive-only weights and remain consistent with the existing if weights.sum() check (keep peak_idx fallback unchanged).
413-419:⚠️ Potential issue | 🔴 CriticalRun the first-grating alignment even without a selector.
The single-detector path is allowed above, but this branch only executes the alignment when
selectoris truthy. Withselector=None,resultsnever gets an entry for the first grating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 413 - 419, The branch currently only runs sparc_auto_grating_offset and populates results when selector is truthy, leaving the first grating out when selector is None; change the logic so selector.moveAbsSync({selector_axes: detector_to_selector[d]}) is only called if selector is not None, but sparc_auto_grating_offset(spectrograph, d) is always invoked and its result stored in future._subfuture/result and results[(g0, d.name)] regardless of selector being None, preserving the mapping keys g0 and d.name.
218-220:⚠️ Potential issue | 🟠 MajorTarget the detector midpoint in the same coordinate system as the peak fit.
find_peak_position()returns zero-based pixel coordinates, so an even-width detector is centered at(width - 1) / 2, notwidth / 2. The current target leaves a perfect alignment 0.5 px off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset.py` around lines 218 - 220, find_peak_position() returns zero-based pixel coords so the detector midpoint should be computed as (width - 1) / 2; update the center_target assignment in goffset.py (the line after estimate_goffset_scale(...) where center_target is set) to use (detector.resolution.value[0] - 1) / 2 (or the equivalent accessor for detector width) instead of detector.resolution.value[0] / 2 so alignments don't end up 0.5 px off.src/odemis/acq/align/test/goffset_alignment_test.py (1)
39-49:⚠️ Potential issue | 🟠 MajorRename or merge the two
test_cancelmethods.Python keeps only the later definition, so one cancellation scenario is silently dropped from the suite.
Also applies to: 68-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_alignment_test.py` around lines 39 - 49, There are two identically named test methods (both named test_cancel) so only the latter runs; rename one (e.g., to test_cancel_immediate or test_cancel_after_sleep) or merge their assertions into a single test to preserve both cancellation scenarios; update references to the future returned by auto_align_grating_detector_offsets, keep the same assertions around f.cancel(), f.cancelled(), and the with self.assertRaises(CancelledError): f.result(...) block, and ensure the timeout decorator (timeout(...)) remains appropriate for the combined/renamed test.src/odemis/acq/align/goffset_alignment.py (3)
17-25:⚠️ Potential issue | 🔴 CriticalUse the cancellation fields this future actually initializes.
auto_align_grating_detector_offsets()sets_task_lock/_task_state, but_checkCancelled()reads_function_lock/_function_state. The first cancellation check will raiseAttributeErrorinstead of stopping cleanly.Suggested fix
-def _checkCancelled(future: "model.ProgressiveFuture"): +def _checkCancelled(future: "model.ProgressiveFuture"): @@ - with future._function_lock: - if future._function_state == CANCELLED: + with future._task_lock: + if future._task_state == CANCELLED: raise CancelledError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset_alignment.py` around lines 17 - 25, _checkCancelled is accessing non-existent attributes _function_lock/_function_state while auto_align_grating_detector_offsets initializes _task_lock/_task_state, causing AttributeError; update _checkCancelled to use the same cancellation fields that the future initializes (use future._task_lock and future._task_state) and keep the same behavior of raising CancelledError when state == CANCELLED so cancellation stops cleanly (or alternatively change the future initialization to set _function_lock/_function_state consistently with _checkCancelled).
145-155:⚠️ Potential issue | 🔴 CriticalRun the first-grating alignment even when
selectorisNone.The public API explicitly allows the single-detector/no-selector case, but this loop only calls
sparc_auto_grating_offset()insideif selector:. In that valid configuration the first grating is never aligned or recorded.Suggested fix
for d in detectors_sorted: _checkCancelled(future) logging.info(f"Starting alignment | Detector: {d.name} | Grating: {g0}") if selector: selector.moveAbsSync({selector_axes: detector_to_selector[d]}) - future._subfuture = sparc_auto_grating_offset(spectrograph, d) - success = future._subfuture.result() - results[(g0, d.name)] = success - - logging.info(f"Finished alignment | Detector: {d.name} | Grating: {g0} | Success: {success}") + future._subfuture = sparc_auto_grating_offset(spectrograph, d) + success = future._subfuture.result() + results[(g0, d.name)] = success + logging.info(f"Finished alignment | Detector: {d.name} | Grating: {g0} | Success: {success}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset_alignment.py` around lines 145 - 155, The loop only calls sparc_auto_grating_offset() when selector is truthy, so in the single-detector/no-selector case the first grating is never aligned or recorded; update the loop in goffset_alignment.py to always invoke sparc_auto_grating_offset(spectrograph, d) and set future._subfuture = that call and results[(g0, d.name)] = success, but keep the selector movement guarded: if selector: selector.moveAbsSync({selector_axes: detector_to_selector[d]}) before calling sparc_auto_grating_offset; preserve the _checkCancelled(future) call and the start/finish logging (including Success) around the alignment so the behavior and logging for detectors_sorted, future, selector_axes, detector_to_selector, sparc_auto_grating_offset, results, g0 and d.name remain consistent.
45-200: 🛠️ Refactor suggestion | 🟠 MajorKeep one source of truth for the orchestration helpers.
src/odemis/acq/align/goffset.pyalready contains the same timing constants and the sameauto_align_grating_detector_offsets()/_do_auto_align_grating_detector_offsets()/_cancel_auto_align_grating_detector_offsets()trio. The copies are already drifting, so bug fixes here will keep missing the other module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/goffset_alignment.py` around lines 45 - 200, Duplicate orchestration helpers and timing constants exist here and in goffset.py, causing drift; remove the copies in this file and import the canonical implementations and constants instead. Replace the local definitions of MOVE_TIME_GRATING, MOVE_TIME_DETECTOR, EST_ALIGN_TIME and the functions auto_align_grating_detector_offsets, _do_auto_align_grating_detector_offsets, _cancel_auto_align_grating_detector_offsets with imports from the single source (goffset.py) so all callers use the same implementation; ensure any local references (e.g., spectrograph, selector, sparc_auto_grating_offset) remain compatible or adapt call sites to the imported functions' signatures. Finally run tests to confirm no behavioral changes.
🧹 Nitpick comments (1)
src/odemis/acq/align/test/goffset_test.py (1)
120-204: Keep the multi-detector alignment suite in one file only.
TestAutoAlignGratingDetectorOffsetsis already implemented insrc/odemis/acq/align/test/goffset_alignment_test.py. Keeping two near-identical copies makes this hardware-heavy flow slower and guarantees the assertions will drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/align/test/goffset_test.py` around lines 120 - 204, This file defines a duplicate TestAutoAlignGratingDetectorOffsets test class that duplicates the implementation in goffset_alignment_test.py; remove the duplicate class (TestAutoAlignGratingDetectorOffsets) or delete the entire file to keep only the single canonical suite in goffset_alignment_test.py, ensuring any unique assertions or setup from this copy are merged into the canonical test (e.g., the tests test_single_detector_iteration, test_multi_detector_iteration, test_cancel and uses of auto_align_grating_detector_offsets, selector, ccd, sp-ccd) before removal so test coverage is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/align/goffset.py`:
- Line 17: The functions lack Python type hints: add explicit parameter and
return type annotations to gaussian (annotate x: Sequence[float] or
numpy.ndarray, amplitude: float, x0: float, width: float, -> numpy.ndarray or
Sequence[float]/float depending on implementation) and to estimate_goffset_scale
(annotate delta: float, retries: int, plus any untyped return type), update
imports (typing or numpy types) if needed, and ensure the signatures for
gaussian and estimate_goffset_scale exactly use the project’s preferred numeric
types (float/int/numpy.ndarray) and include return type hints to satisfy the
coding guideline.
- Around line 308-311: The type annotation uses a forward reference
List['Stream'] in auto_align_grating_detector_offsets (and the other occurrences
around line 360) but Stream is not imported, causing an F821 undefined name; fix
by adding the import "from odemis.acq.stream import Stream" at the top of the
module so the Stream name is defined for the annotations (or alternatively
convert the string annotations to typing.TYPE_CHECKING guarded import), ensuring
auto_align_grating_detector_offsets and related signatures resolve correctly.
In `@src/odemis/acq/align/test/goffset_alignment_test.py`:
- Around line 123-125: Remove the trailing whitespace on the final line that
contains the "__name__ == '__main__'" block and ensure the file ends with a
single newline; specifically edit the lines with "if __name__ == '__main__':"
and "unittest.main()" to remove any trailing spaces and add a final newline
character, then run autopep8 (autopep8 --in-place --select W291,W292,W293,W391)
to automatically fix W293/W292 lint warnings.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 117-118: The file src/odemis/acq/align/test/goffset_test.py
contains trailing whitespace / whitespace-only blank lines around the area
indicated (causing W293); remove those blank lines (or strip trailing spaces) so
no whitespace-only lines remain, then reformat the file with autopep8 (autopep8
--in-place --select W291,W292,W293,W391) to ensure the W293 lint error is
cleared; look for the blank lines in the goffset_test.py test module near the
reported location to make the change.
---
Duplicate comments:
In `@src/odemis/acq/align/goffset_alignment.py`:
- Around line 17-25: _checkCancelled is accessing non-existent attributes
_function_lock/_function_state while auto_align_grating_detector_offsets
initializes _task_lock/_task_state, causing AttributeError; update
_checkCancelled to use the same cancellation fields that the future initializes
(use future._task_lock and future._task_state) and keep the same behavior of
raising CancelledError when state == CANCELLED so cancellation stops cleanly (or
alternatively change the future initialization to set
_function_lock/_function_state consistently with _checkCancelled).
- Around line 145-155: The loop only calls sparc_auto_grating_offset() when
selector is truthy, so in the single-detector/no-selector case the first grating
is never aligned or recorded; update the loop in goffset_alignment.py to always
invoke sparc_auto_grating_offset(spectrograph, d) and set future._subfuture =
that call and results[(g0, d.name)] = success, but keep the selector movement
guarded: if selector: selector.moveAbsSync({selector_axes:
detector_to_selector[d]}) before calling sparc_auto_grating_offset; preserve the
_checkCancelled(future) call and the start/finish logging (including Success)
around the alignment so the behavior and logging for detectors_sorted, future,
selector_axes, detector_to_selector, sparc_auto_grating_offset, results, g0 and
d.name remain consistent.
- Around line 45-200: Duplicate orchestration helpers and timing constants exist
here and in goffset.py, causing drift; remove the copies in this file and import
the canonical implementations and constants instead. Replace the local
definitions of MOVE_TIME_GRATING, MOVE_TIME_DETECTOR, EST_ALIGN_TIME and the
functions auto_align_grating_detector_offsets,
_do_auto_align_grating_detector_offsets,
_cancel_auto_align_grating_detector_offsets with imports from the single source
(goffset.py) so all callers use the same implementation; ensure any local
references (e.g., spectrograph, selector, sparc_auto_grating_offset) remain
compatible or adapt call sites to the imported functions' signatures. Finally
run tests to confirm no behavioral changes.
In `@src/odemis/acq/align/goffset.py`:
- Around line 127-133: The temporary spectrograph move using
spgr.moveRelSync({"goffset": actual_delta}) must be reverted inside a finally
block so the rollback always runs even if detector.data.get or
find_peak_position raise; wrap the "move and measure" sequence
(spgr.moveRelSync({"goffset": actual_delta}), data1 =
detector.data.get(asap=False), p1 = find_peak_position(data1)) in try and
perform spgr.moveRelSync({"goffset": -actual_delta}) in a finally clause,
preserving any raised exceptions (re-raise if needed) so the spectrograph is
always returned to the start position.
- Around line 271-278: The canceller _cancel_sparc_auto_grating_offset currently
sets future._task_state = CANCELLED under future._task_lock but returns None,
which makes ProgressiveFuture.cancel() see a falsy result; update
_cancel_sparc_auto_grating_offset to return a truthy value (e.g., True) after
successfully acquiring the lock and setting future._task_state, ensuring
ProgressiveFuture.cancel() observes a successful cancellation; keep the lock
usage and state assignment as-is and simply add the boolean return at the end.
- Around line 64-72: The centroid fallback is inconsistent: the guard checks
weights.sum() but the centroid uses window_data, allowing negative baseline
values to skew the result; change the centroid computation for weighted_avg to
use the clipped positive weights (use numpy.sum(window_idx * weights) /
numpy.sum(weights)) so the numerator and denominator reference the same
positive-only weights and remain consistent with the existing if weights.sum()
check (keep peak_idx fallback unchanged).
- Around line 413-419: The branch currently only runs sparc_auto_grating_offset
and populates results when selector is truthy, leaving the first grating out
when selector is None; change the logic so selector.moveAbsSync({selector_axes:
detector_to_selector[d]}) is only called if selector is not None, but
sparc_auto_grating_offset(spectrograph, d) is always invoked and its result
stored in future._subfuture/result and results[(g0, d.name)] regardless of
selector being None, preserving the mapping keys g0 and d.name.
- Around line 218-220: find_peak_position() returns zero-based pixel coords so
the detector midpoint should be computed as (width - 1) / 2; update the
center_target assignment in goffset.py (the line after
estimate_goffset_scale(...) where center_target is set) to use
(detector.resolution.value[0] - 1) / 2 (or the equivalent accessor for detector
width) instead of detector.resolution.value[0] / 2 so alignments don't end up
0.5 px off.
In `@src/odemis/acq/align/test/goffset_alignment_test.py`:
- Around line 39-49: There are two identically named test methods (both named
test_cancel) so only the latter runs; rename one (e.g., to test_cancel_immediate
or test_cancel_after_sleep) or merge their assertions into a single test to
preserve both cancellation scenarios; update references to the future returned
by auto_align_grating_detector_offsets, keep the same assertions around
f.cancel(), f.cancelled(), and the with self.assertRaises(CancelledError):
f.result(...) block, and ensure the timeout decorator (timeout(...)) remains
appropriate for the combined/renamed test.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 43-49: Uncomment and re-enable the tearDownClass cleanup so the
spectrograph state is restored: restore the commented-out `@classmethod`
tearDownClass in the test class, call
cls.spgr.moveAbsSync(cls._original_position) inside a try/except, and on
exception call logging.exception("Failed restoring spectrograph position") so
that the spectrograph (cls.spgr) is returned to cls._original_position after the
suite runs.
- Around line 83-87: The test is calling estimate_goffset_scale(self.spgr,
self.detector) but that function returns a tuple (scale, p0, p1); update the
test in goffset_test.py to unpack the returned tuple (e.g. scale, p0, p1 =
estimate_goffset_scale(...)) and then run the existing assertions against scale
(assertIsInstance(scale, float), assertNotEqual(scale, 0.0),
assertTrue(np.isfinite(scale))). Ensure you reference estimate_goffset_scale and
the local variables p0/p1 when making the change.
- Around line 94-99: The test sets delta = 0 so the relative move is a no-op;
change the test in goffset_test.py to use a non-zero delta (e.g., a small amount
appropriate for the system) instead of 0, keeping the existing computation of
current = self.spgr.position.value["goffset"], goffset_max =
self.spgr.axes["goffset"].range[1], direction = 1 if (current + delta <
goffset_max) else -1 and then calling self.spgr.moveRelSync({"goffset": delta *
direction}) so the test actually introduces a misalignment to verify recovery.
- Around line 110-116: The test currently swallows all exceptions when waiting
on the future returned by sparc_auto_grating_offset(self.spgr, self.detector);
change the try/except so you only catch the timeout, call f.cancel() when a
timeout occurs, and let other exceptions propagate. Specifically, replace the
broad except with catching concurrent.futures.TimeoutError (or
asyncio.TimeoutError if using asyncio futures), call f.cancel() inside that
except block, and avoid catching Exception or bare except so real failures still
surface; keep the final assertion self.assertTrue(f.done()) to verify the future
was finished or cancelled.
- Line 22: The module is calling logging.getLogger().setLevel(logging.DEBUG) at
import time which mutates the root logger and leaks DEBUG into other tests;
remove that import-time root-level change and instead obtain a module logger via
logger = logging.getLogger(__name__) and use that logger for local debug calls,
or move any test-specific logging configuration into the test setup/teardown (or
an if __name__ == '__main__' guard) so you do not set the root logger level
during import.
---
Nitpick comments:
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 120-204: This file defines a duplicate
TestAutoAlignGratingDetectorOffsets test class that duplicates the
implementation in goffset_alignment_test.py; remove the duplicate class
(TestAutoAlignGratingDetectorOffsets) or delete the entire file to keep only the
single canonical suite in goffset_alignment_test.py, ensuring any unique
assertions or setup from this copy are merged into the canonical test (e.g., the
tests test_single_detector_iteration, test_multi_detector_iteration, test_cancel
and uses of auto_align_grating_detector_offsets, selector, ccd, sp-ccd) before
removal so test coverage is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 094c64f4-6487-4331-907b-6ecb9a601c52
📒 Files selected for processing (4)
src/odemis/acq/align/goffset.pysrc/odemis/acq/align/goffset_alignment.pysrc/odemis/acq/align/test/goffset_alignment_test.pysrc/odemis/acq/align/test/goffset_test.py
| from odemis.util import executeAsyncTask | ||
| from scipy.optimize import curve_fit | ||
|
|
||
| def gaussian(x, amplitude, x0, width): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Finish annotating the remaining helper signatures.
gaussian() is still untyped, and estimate_goffset_scale() leaves delta and retries unannotated. As per coding guidelines, "Always use type hints for function parameters and return types in Python code".
Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/align/goffset.py` at line 17, The functions lack Python type
hints: add explicit parameter and return type annotations to gaussian (annotate
x: Sequence[float] or numpy.ndarray, amplitude: float, x0: float, width: float,
-> numpy.ndarray or Sequence[float]/float depending on implementation) and to
estimate_goffset_scale (annotate delta: float, retries: int, plus any untyped
return type), update imports (typing or numpy types) if needed, and ensure the
signatures for gaussian and estimate_goffset_scale exactly use the project’s
preferred numeric types (float/int/numpy.ndarray) and include return type hints
to satisfy the coding guideline.
de03ff2 to
af6f838
Compare
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
from
af6f838 to
37c53b3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.