[feat] add peak-finding algorithm#3395
Open
yxkdejong wants to merge 11 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a spectral peak alignment workflow for grating–detector offset calibration. A new module implements gaussian modeling, peak finding, peak presence checks, coarse goffset scanning, goffset-to-pixel scale estimation, and an asynchronous iterative alignment task (sparc_auto_grating_offset). A separate orchestrator (auto_align_grating_detector_offsets and goffset_alignment helpers) runs multi-grating/multi-detector sequences with selector support, cancellation, progress reporting, and time estimates. New unit and integration tests and GUI wiring for an Auto-Calibration button are included. 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
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
![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
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
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
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
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
from
de03ff2 to
af6f838
Compare
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
from
af6f838 to
37c53b3
Compare
tepals
requested changes
Mar 30, 2026
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
2 times, most recently
from
fc43a4a to
d03d78d
Compare
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
from
d03d78d to
9670b75
Compare
yxkdejong
force-pushed
the
add-automated-grating-alignment
branch
from
11d5873 to
d58b74f
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (11)
src/odemis/acq/align/goffset_alignment.py (3)
17-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the fields this future actually initializes.
auto_align_grating_detector_offsets()sets_task_lockand_task_state, but_checkCancelled()reads_function_lockand_function_state. The first cancellation check will raiseAttributeErrorinstead of cancelling cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset_alignment.py` around lines 17 - 25, The cancellation checker _checkCancelled reads non-existent attributes _function_lock/_function_state causing AttributeError; update it to use the actual fields initialized by the future—use future._task_lock and future._task_state (and keep the same CANCELLED comparison and CancelledError raise) so auto_align_grating_detector_offsets() can cancel cleanly.
149-155:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRun the first-grating alignment even when
selectorisNone.The single-detector/no-selector case is explicitly allowed above, but this loop only calls
sparc_auto_grating_offset()inside theif selector:branch. In that valid configuration the first grating never gets aligned or stored inresults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset_alignment.py` around lines 149 - 155, The loop currently only calls sparc_auto_grating_offset and stores its result when selector is truthy, so in the allowed single-detector/no-selector case the first grating (g0) never gets aligned or added to results; change the logic so sparc_auto_grating_offset(spectrograph, d) is invoked and awaited (via future._subfuture and .result()) regardless of selector, but only call selector.moveAbsSync when selector is not None; ensure results[(g0, d.name)] = success and the logging.info line run in both branches so the alignment result for g0 is always recorded.
45-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the
Streamforward references.Ruff is already reporting F821 for both annotations in this file. Add a guarded import so these signatures stop failing lint/type checks.
Also applies to: 93-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset_alignment.py` around lines 45 - 48, The annotations use the forward reference 'Stream' and cause F821; add a guarded import using typing.TYPE_CHECKING and import Stream inside the TYPE_CHECKING block so the runtime is unchanged but the name is available for linting/typing. Specifically, add "from typing import TYPE_CHECKING" and then "if TYPE_CHECKING: from <appropriate_module> import Stream" (replace <appropriate_module> with the module that defines Stream) near the top of the file, and ensure the same guarded import covers the other functions using 'Stream' (e.g., auto_align_grating_detector_offsets and the functions around lines 93–97) so their annotations no longer raise F821.src/odemis/acq/align/test/goffset_test.py (4)
488-494:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActually exercise the cancellation path here.
This test waits on
result()and swallows every exception, so it can pass on normal completion too. Callcancel()and assertresult()raisesCancelledError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 488 - 494, The test currently waits on the future returned by sparc_auto_grating_offset(self.spgr, self.detector) and swallows all exceptions, so it doesn't verify cancellation; modify the test to call cancel() on the future (f.cancel()) after creating it, then assert that f.cancelled() is True (or f.done()) and that calling f.result() raises the appropriate CancelledError (use the async future/asyncio CancelledError or concurrent.futures.CancelledError depending on which Future type sparc_auto_grating_offset returns) instead of silently catching all exceptions.
44-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the spectrograph state in
tearDownClass().This suite moves the spectrograph repeatedly, but cleanup is commented out. That makes later tests order-dependent and leaves the backend mutated after this file finishes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 44 - 50, Uncomment and re-enable the tearDownClass class method so the test suite restores spectrograph state after tests: implement a `@classmethod` tearDownClass(cls) that calls cls.spgr.moveAbsSync(cls._original_position) inside a try/except, logging any exception with logging.exception("Failed restoring spectrograph position") to ensure the backend is returned to its original position and tests remain order-independent; reference the existing cls.spgr, cls._original_position, and tearDownClass identifiers when restoring state.
82-90:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnpack
estimate_goffset_scale()before asserting onscale.The helper returns
(scale, p0, p1). As written,scaleis the full tuple, soassertIsInstance(scale, float)will fail immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 82 - 90, The test currently treats the return of estimate_goffset_scale(self.spgr, self.detector) as a float but the function returns a tuple (scale, p0, p1); update test_estimate_goffset_scale to unpack the return like (scale, p0, p1) = estimate_goffset_scale(self.spgr, self.detector) and then run the existing assertions (assertIsInstance(scale, float), assertNotEqual(scale, 0.0), assertTrue(np.isfinite(scale))) against the unpacked scale variable.
147-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis test never creates the misalignment it claims to verify.
delta = 0makes the move a no-op, so the test can pass even if recovery from a real offset regression is broken.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 147 - 156, The test sets delta = 0 so no misalignment occurs; change delta to a non-zero value (e.g., a small positive int/float) so the move actually shifts goffset, keep the existing calculation of current = self.spgr.position.value["goffset"], goffset_max = self.spgr.axes["goffset"].range[1], and direction = 1 if (current + delta < goffset_max) else -1, then call self.spgr.moveRelSync({"goffset": delta * direction}) as before so the test produces a real misalignment to verify recovery.src/odemis/acq/align/goffset.py (4)
81-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the clipped weights consistently in the centroid fallback.
The denominator uses
weights, but the numerator still useswindow_data. Negative baseline samples can still pull the fallback estimate away from the positive lobe you already clipped for the zero-sum check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset.py` around lines 81 - 89, The centroid fallback uses a clipped weights array but still multiplies window_idx by the original window_data; change the numerator to use the clipped weights so the centroid uses only non-negative contributions: when computing weighted_avg in the else branch replace the numerator window_idx * window_data with window_idx * weights (refer to variables weights, window_data, window_idx, weighted_avg and peak_idx) so the weighted average is computed as sum(window_idx * weights)/sum(weights).
566-569:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the
Streamforward references.Ruff is already flagging both
List['Stream']annotations as F821 here. Add a guarded import or a direct import so these signatures stop failing lint/type checks.Also applies to: 610-614
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset.py` around lines 566 - 569, The F821 errors come from using the forward-reference type List['Stream'] in the signatures of auto_align_grating_detector_offsets (and the similar function at ~610-614); fix by adding a guarded import: import TYPE_CHECKING from typing and inside if TYPE_CHECKING: from <appropriate_module> import Stream (or the correct module/class name where Stream is defined), so the Stream symbol is available for type checkers but not at runtime; update both function signatures locations to rely on that guarded import (or alternatively enable postponed evaluation via from __future__ import annotations) to resolve the lint/type errors.
532-537:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn a boolean from
_cancel_sparc_auto_grating_offset().This helper is annotated as
-> bool, but it returnsNone. That makes it impossible for the outer future to observe a successful cancellation through the task canceller contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset.py` around lines 532 - 537, The function _cancel_sparc_auto_grating_offset currently sets future._task_state under future._task_lock but does not return any value; change it to return a boolean indicating success of the cancellation (e.g., after acquiring future._task_lock and setting future._task_state = CANCELLED, return True). Ensure you keep the existing lock usage and symbol names (future._task_lock, future._task_state, CANCELLED) so callers relying on the canceller contract receive a True/False result.
272-278:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways roll back the probe move in
estimate_goffset_scale().If
detector.data.get()orfind_peak_position()raises after the temporarymoveRelSync, the spectrograph stays at the probe offset and the rest of the alignment continues from the wrong hardware state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset.py` around lines 272 - 278, The temporary probe move in estimate_goffset_scale() must always be rolled back even if detector.data.get() or find_peak_position() throws; wrap the move-and-measure sequence (spgr.moveRelSync({"goffset": actual_delta}), detector.data.get(...), find_peak_position(...)) in a try/finally and perform spgr.moveRelSync({"goffset": -actual_delta}) in the finally block so the spectrograph state is restored regardless of errors, keeping existing variables like actual_delta and using the same spgr.moveRelSync call for rollback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/acq/align/test/goffset_alignment_test.py`:
- Around line 17-23: The test top-level import list references acquire_peak
which no longer exists in odemis.acq.align.goffset; update the import line in
goffset_alignment_test.py to remove acquire_peak (or replace it with the current
equivalent function name if renamed) so the module imports only existing symbols
(e.g., keep find_peak_position, estimate_goffset_scale,
sparc_auto_grating_offset, auto_align_grating_detector_offsets or the correct
replacement); ensure the import names exactly match the function names defined
in odemis.acq.align.goffset to avoid ImportError during test discovery.
- Around line 10-28: The test uses odemis.__file__ when building CONFIG_PATH but
only does "from odemis import model, acq", so add an explicit "import odemis"
(or change CONFIG_PATH to use model.__file__) so the package name is bound;
update the imports at the top to include "import odemis" and leave CONFIG_PATH
and SPARC_CONFIG intact, ensuring references to CONFIG_PATH, SPARC_CONFIG, and
the existing imported symbols (model, acq) remain correct.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 318-338: Replace uses of the non-existent attribute self.ccd with
the fixture-created self.detector: update the detectors list (currently
"detectors = [self.ccd, spccd]") to use self.detector, and update the membership
assertions that reference self.ccd.name to reference self.detector.name so the
test uses the correct fixture-created detector in
auto_align_grating_detector_offsets and subsequent checks.
- Line 180: The test calls sparc_auto_grating_offset with an unsupported keyword
single_detector_mode causing a TypeError; remove that argument (or replace it
with a supported parameter if intended) in the test invocation in
goffset_test.py so the call matches the function signature of
sparc_auto_grating_offset and lets the test execute normally.
- Around line 25-27: The test uses HOME_PATH and H5_FILE_2d_NO_PEAK hardcoded to
a user-specific home path; change these to repository-relative pathlib.Path
references by replacing HOME_PATH and H5_FILE_2d_NO_PEAK with a Path built from
the test file location (e.g. Path(__file__).resolve().parent) and then join the
relative fixture path using the / operator (e.g. Path(__file__).resolve().parent
/ "relative" / "path" / "to" / "grating 2 1024x256"); ensure H5_FILE_2d_NO_PEAK
is a pathlib.Path (not an os.path string) so downstream code uses Path methods
or cast to str only where required.
- Line 352: The test contains an incorrect assertion using the bound method
data.min instead of calling it, so update the assertion in goffset_test.py to
call data.min(): replace self.assertNotEqual(data.max(), data.min) with
self.assertNotEqual(data.max(), data.min()). Ensure any similar flat-image
checks in the same test function or nearby assertions also call .min() (and
.max() if needed) rather than referencing the method object.
In `@src/odemis/gui/cont/tabs/sparc2_align_tab.py`:
- Around line 1562-1564: The code creates and binds a new wx.Timer every time
the start path runs (creating multiple timers and EVT_TIMER handlers); modify
the logic so _progress_timer is allocated and bound only once (e.g., initialize
and Bind self._progress_timer to self._on_progress_timer in the constructor or
startup initialization), and change the start/cancel paths to reuse the existing
timer by calling Start/Stop on self._progress_timer (and call Stop and
optionally Destroy/unbind on explicit teardown) instead of creating a new
wx.Timer each click; update references to self._progress_timer, panel.Bind,
_on_progress_timer and Start/Stop accordingly.
- Line 48: The import path odemis.acq.align.goffset_ext is invalid; update the
import in sparc2_align_tab.py to import auto_align_grating_detector_offsets from
the actual module that defines it (either odemis.acq.align.goffset or
odemis.acq.align.goffset_alignment) — replace the faulty import with a direct
import of auto_align_grating_detector_offsets from the correct module (the one
where the function is implemented) so the tab can import at startup.
- Around line 1621-1636: The _on_auto_calibrate_done callback must stop the wx
timer on the main thread and include type hints and a reStructuredText
docstring: wrap the existing self._progress_timer.Stop() call in wx.CallAfter
(as you already do for SetLabel/SetValue) to ensure thread safety when accessing
the wx.Timer named _progress_timer; add type hints for the parameter and return
(e.g., def _on_auto_calibrate_done(self, f: "concurrent.futures.Future") ->
None) and add a short rst-style docstring explaining this callback handles the
auto-calibration completion and updates UI elements (referencing method name
_on_auto_calibrate_done and attributes _progress_timer,
panel.btn_auto_calibrate, panel.gauge_auto_calibrate).
---
Duplicate comments:
In `@src/odemis/acq/align/goffset_alignment.py`:
- Around line 17-25: The cancellation checker _checkCancelled reads non-existent
attributes _function_lock/_function_state causing AttributeError; update it to
use the actual fields initialized by the future—use future._task_lock and
future._task_state (and keep the same CANCELLED comparison and CancelledError
raise) so auto_align_grating_detector_offsets() can cancel cleanly.
- Around line 149-155: The loop currently only calls sparc_auto_grating_offset
and stores its result when selector is truthy, so in the allowed
single-detector/no-selector case the first grating (g0) never gets aligned or
added to results; change the logic so sparc_auto_grating_offset(spectrograph, d)
is invoked and awaited (via future._subfuture and .result()) regardless of
selector, but only call selector.moveAbsSync when selector is not None; ensure
results[(g0, d.name)] = success and the logging.info line run in both branches
so the alignment result for g0 is always recorded.
- Around line 45-48: The annotations use the forward reference 'Stream' and
cause F821; add a guarded import using typing.TYPE_CHECKING and import Stream
inside the TYPE_CHECKING block so the runtime is unchanged but the name is
available for linting/typing. Specifically, add "from typing import
TYPE_CHECKING" and then "if TYPE_CHECKING: from <appropriate_module> import
Stream" (replace <appropriate_module> with the module that defines Stream) near
the top of the file, and ensure the same guarded import covers the other
functions using 'Stream' (e.g., auto_align_grating_detector_offsets and the
functions around lines 93–97) so their annotations no longer raise F821.
In `@src/odemis/acq/align/goffset.py`:
- Around line 81-89: The centroid fallback uses a clipped weights array but
still multiplies window_idx by the original window_data; change the numerator to
use the clipped weights so the centroid uses only non-negative contributions:
when computing weighted_avg in the else branch replace the numerator window_idx
* window_data with window_idx * weights (refer to variables weights,
window_data, window_idx, weighted_avg and peak_idx) so the weighted average is
computed as sum(window_idx * weights)/sum(weights).
- Around line 566-569: The F821 errors come from using the forward-reference
type List['Stream'] in the signatures of auto_align_grating_detector_offsets
(and the similar function at ~610-614); fix by adding a guarded import: import
TYPE_CHECKING from typing and inside if TYPE_CHECKING: from <appropriate_module>
import Stream (or the correct module/class name where Stream is defined), so the
Stream symbol is available for type checkers but not at runtime; update both
function signatures locations to rely on that guarded import (or alternatively
enable postponed evaluation via from __future__ import annotations) to resolve
the lint/type errors.
- Around line 532-537: The function _cancel_sparc_auto_grating_offset currently
sets future._task_state under future._task_lock but does not return any value;
change it to return a boolean indicating success of the cancellation (e.g.,
after acquiring future._task_lock and setting future._task_state = CANCELLED,
return True). Ensure you keep the existing lock usage and symbol names
(future._task_lock, future._task_state, CANCELLED) so callers relying on the
canceller contract receive a True/False result.
- Around line 272-278: The temporary probe move in estimate_goffset_scale() must
always be rolled back even if detector.data.get() or find_peak_position()
throws; wrap the move-and-measure sequence (spgr.moveRelSync({"goffset":
actual_delta}), detector.data.get(...), find_peak_position(...)) in a
try/finally and perform spgr.moveRelSync({"goffset": -actual_delta}) in the
finally block so the spectrograph state is restored regardless of errors,
keeping existing variables like actual_delta and using the same spgr.moveRelSync
call for rollback.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 488-494: The test currently waits on the future returned by
sparc_auto_grating_offset(self.spgr, self.detector) and swallows all exceptions,
so it doesn't verify cancellation; modify the test to call cancel() on the
future (f.cancel()) after creating it, then assert that f.cancelled() is True
(or f.done()) and that calling f.result() raises the appropriate CancelledError
(use the async future/asyncio CancelledError or
concurrent.futures.CancelledError depending on which Future type
sparc_auto_grating_offset returns) instead of silently catching all exceptions.
- Around line 44-50: Uncomment and re-enable the tearDownClass class method so
the test suite restores spectrograph state after tests: implement a `@classmethod`
tearDownClass(cls) that calls cls.spgr.moveAbsSync(cls._original_position)
inside a try/except, logging any exception with logging.exception("Failed
restoring spectrograph position") to ensure the backend is returned to its
original position and tests remain order-independent; reference the existing
cls.spgr, cls._original_position, and tearDownClass identifiers when restoring
state.
- Around line 82-90: The test currently treats the return of
estimate_goffset_scale(self.spgr, self.detector) as a float but the function
returns a tuple (scale, p0, p1); update test_estimate_goffset_scale to unpack
the return like (scale, p0, p1) = estimate_goffset_scale(self.spgr,
self.detector) and then run the existing assertions (assertIsInstance(scale,
float), assertNotEqual(scale, 0.0), assertTrue(np.isfinite(scale))) against the
unpacked scale variable.
- Around line 147-156: The test sets delta = 0 so no misalignment occurs; change
delta to a non-zero value (e.g., a small positive int/float) so the move
actually shifts goffset, keep the existing calculation of current =
self.spgr.position.value["goffset"], goffset_max =
self.spgr.axes["goffset"].range[1], and direction = 1 if (current + delta <
goffset_max) else -1, then call self.spgr.moveRelSync({"goffset": delta *
direction}) as before so the test produces a real misalignment to verify
recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7021f94-b4a6-4453-a5ca-13c7623d6a64
📒 Files selected for processing (5)
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.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.py
Comment on lines
+25
to
+27
| HOME_PATH = os.path.expanduser("~") + "/" | ||
| H5_FILE_2d_NO_PEAK = HOME_PATH + "development/odemis/grating 2 1024x256/" | ||
|
|
There was a problem hiding this comment.
Make the HDF5 fixtures repository-relative.
~/development/odemis/... only exists on one workstation, so this test will fail in CI and on other dev machines before it even reaches the assertions. Resolve the fixtures from the test file location instead.
As per coding guidelines, "For file paths, use pathlib.Path, unless interfacing with existing code that uses os.path strings".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/acq/align/test/goffset_test.py` around lines 25 - 27, The test
uses HOME_PATH and H5_FILE_2d_NO_PEAK hardcoded to a user-specific home path;
change these to repository-relative pathlib.Path references by replacing
HOME_PATH and H5_FILE_2d_NO_PEAK with a Path built from the test file location
(e.g. Path(__file__).resolve().parent) and then join the relative fixture path
using the / operator (e.g. Path(__file__).resolve().parent / "relative" / "path"
/ "to" / "grating 2 1024x256"); ensure H5_FILE_2d_NO_PEAK is a pathlib.Path (not
an os.path string) so downstream code uses Path methods or cast to str only
where required.
d58b74f to
39c44f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
src/odemis/acq/align/test/goffset_test.py (6)
25-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse repository-relative fixture paths instead of
~/development/....These fixtures only exist on one workstation, so both HDF5 tests fail before they exercise the code under test. Resolve them from
Path(__file__)instead of a hardcoded home-directory path. As per coding guidelines, "For file paths, use pathlib.Path, unless interfacing with existing code that uses os.path strings".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 25 - 26, Replace the hardcoded HOME_PATH and H5_FILE_2d_NO_PEAK string construction with repository-relative pathlib resolution: use Path(__file__).resolve().parent (or navigate from that directory) to build the HDF5 fixture path and assign it to H5_FILE_2d_NO_PEAK as a Path object (or convert to str where needed); update any code using HOME_PATH/H5_FILE_2d_NO_PEAK to use the new Path-based value so tests no longer rely on ~/development/... and follow the project guideline to prefer pathlib.
552-558:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActually exercise the cancellation path.
This test waits on
result()and swallows every exception, so it can pass on normal completion, timeout, or unrelated failures. Callcancel()and assert thatresult()raisesCancelledError.Possible fix
def test_cancel(self): """ Test cancelling alignment. """ 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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 552 - 558, The test currently swallows all exceptions from waiting on the future returned by sparc_auto_grating_offset(self.spgr, self.detector) so it doesn't verify cancellation; modify the test to explicitly cancel the future (call f.cancel()), then assert the future reports cancellation (e.g., f.cancelled() or f.done()) and that calling f.result() raises asyncio.CancelledError (use an assertion like assertRaises or context manager for CancelledError) instead of a blanket try/except that swallows errors.
44-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-enable suite cleanup for the spectrograph state.
Leaving
tearDownClass()commented out makes these hardware tests order-dependent and leaks the modified spectrograph position into later suites.Possible fix
- # `@classmethod` - # def tearDownClass(cls): - # # restore original position - # try: - # cls.spgr.moveAbsSync(cls._original_position) - # except Exception: - # logging.exception("Failed restoring spectrograph position") + `@classmethod` + def tearDownClass(cls): + # restore original position + try: + cls.spgr.moveAbsSync(cls._original_position) + except Exception: + logging.exception("Failed restoring spectrograph position")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 44 - 50, Uncomment and re-enable the class-level cleanup method tearDownClass so the spectrograph position is restored after the test suite: implement/restore the previously commented tearDownClass that calls cls.spgr.moveAbsSync(cls._original_position) inside a try/except and uses logging.exception("Failed restoring spectrograph position") on failure; ensure cls._original_position is captured during setUpClass and that cls.spgr is available before attempting the move to avoid AttributeError.
86-90:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnpack
estimate_goffset_scale()before asserting onscale.The helper returns
(scale, p0, p1), soscaleis a tuple here andassertIsInstance(scale, float)will fail immediately.Possible 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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 86 - 90, The test is treating the return of estimate_goffset_scale(self.spgr, self.detector) as a float but the function returns a tuple (scale, p0, p1); update the call to unpack the 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 names scale, p0, p1 when making the change.
337-337:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
self.detector, not undefinedself.ccd.
setUpClass()createsself.detector; this assertion will raiseAttributeErroras written.Possible fix
- self.assertIn(self.ccd.name, dets_for_first_grating) + self.assertIn(self.detector.name, dets_for_first_grating)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` at line 337, The test uses an undefined attribute self.ccd causing AttributeError; change the assertion in the test (the line asserting membership of detector name in dets_for_first_grating) to reference the correctly initialized attribute self.detector (i.e., use self.detector.name) so the test uses the object created in setUpClass() instead of the nonexistent self.ccd.
147-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis test never creates the misalignment it claims to verify.
delta = 0makes Line 155 a no-op, so the test can pass even if recovery from a real misalignment regresses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/test/goffset_test.py` around lines 147 - 155, The test sets delta = 0 so no misalignment occurs; change delta to a non-zero value (for example 1 or the goffset axis step like self.spgr.axes["goffset"].step) so the subsequent spgr.moveRelSync({"goffset": delta * direction}) actually moves the axis and creates the intended misalignment; keep the existing bounds check using current, goffset_max and direction to avoid moving out of range.src/odemis/acq/align/goffset.py (3)
533-538:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn a boolean from
_cancel_sparc_auto_grating_offset().This canceller currently returns
None, so callers can observecancel()as a failure even though_task_statewas flipped toCANCELLED.Possible fix
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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset.py` around lines 533 - 538, The canceller _cancel_sparc_auto_grating_offset currently sets future._task_state to CANCELLED under future._task_lock but returns None; change it to return a boolean success value (e.g., return True) after successfully setting the state so callers see cancellation succeeded. Locate _cancel_sparc_auto_grating_offset (signature: def _cancel_sparc_auto_grating_offset(future: model.ProgressiveFuture) -> bool), keep the locking and state assignment using future._task_lock and CANCELLED, then add a return True at the end (and optionally return False on any early-failure path if added).
273-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways roll back the probe move in
estimate_goffset_scale().If
detector.data.get()orfind_peak_position()raises after Line 274, the spectrograph stays at the temporary test offset and the next calibration step starts from mutated hardware state.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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset.py` around lines 273 - 279, In estimate_goffset_scale(), ensure the temporary probe move via spgr.moveRelSync({"goffset": actual_delta}) is always undone even if detector.data.get() or find_peak_position() raises: wrap the measurement steps (detector.data.get, find_peak_position) in a try/finally and call spgr.moveRelSync({"goffset": -actual_delta}) in the finally block so the spectrograph is restored to its original goffset before re-raising or returning; reference the existing symbols estimate_goffset_scale, spgr.moveRelSync, detector.data.get, and find_peak_position to locate where to add the try/finally.
84-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the clipped weights consistently in the centroid fallback.
Line 90 still mixes
window_datain the numerator withweightsin the denominator. If the fitting window contains negative baseline values, the fallback centroid can shift away from the actual positive lobe and steer the alignment the wrong way.Possible fix
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(weights)) + weighted_avg = float(numpy.sum(window_idx * weights) / numpy.sum(weights))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/align/goffset.py` around lines 84 - 90, The centroid fallback mixes raw window_data in the numerator with clipped weights in the denominator; change the numerator to use the same clipped weights (i.e., compute weighted_avg = float(numpy.sum(window_idx * weights) / numpy.sum(weights)) or window_idx * clipped_weights) so both numerator and denominator use the same weights variable (referencing weights, window_idx, peak_idx, weighted_avg) and preserve the zero-sum check that sets weighted_avg = float(peak_idx) when weights.sum() == 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/acq/align/goffset.py`:
- Around line 766-778: The finally block must make hardware restores
best-effort: wrap the spectrograph.restore call
(spectrograph.moveAbsSync(original_pos)) in its own try/except that catches
Exception and calls logging.exception("Failed to restore spectrograph position
during alignment cleanup"); do the same for
selector.moveAbsSync(original_selector) only if selector is truthy, logging a
selector-specific message; ensure these try/excepts prevent exceptions from
propagating so the subsequent with future._task_lock: future._task_state =
FINISHED always executes (i.e., leave the FINISHED assignment outside those try
blocks or in its own guaranteed path).
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 279-280: The test calls to auto_align_grating_detector_offsets no
longer match its signature; update each invocation (e.g., the calls on lines
referenced) to pass the required arguments opm, align_mode, and bl — for example
provide the test's corresponding objects (e.g., self.opm, self.align_mode,
self.bl or the proper fixtures) along with spectrograph=self.spgr,
detectors=self.detector, selector=self.selector so the function is invoked with
the new parameters.
---
Duplicate comments:
In `@src/odemis/acq/align/goffset.py`:
- Around line 533-538: The canceller _cancel_sparc_auto_grating_offset currently
sets future._task_state to CANCELLED under future._task_lock but returns None;
change it to return a boolean success value (e.g., return True) after
successfully setting the state so callers see cancellation succeeded. Locate
_cancel_sparc_auto_grating_offset (signature: def
_cancel_sparc_auto_grating_offset(future: model.ProgressiveFuture) -> bool),
keep the locking and state assignment using future._task_lock and CANCELLED,
then add a return True at the end (and optionally return False on any
early-failure path if added).
- Around line 273-279: In estimate_goffset_scale(), ensure the temporary probe
move via spgr.moveRelSync({"goffset": actual_delta}) is always undone even if
detector.data.get() or find_peak_position() raises: wrap the measurement steps
(detector.data.get, find_peak_position) in a try/finally and call
spgr.moveRelSync({"goffset": -actual_delta}) in the finally block so the
spectrograph is restored to its original goffset before re-raising or returning;
reference the existing symbols estimate_goffset_scale, spgr.moveRelSync,
detector.data.get, and find_peak_position to locate where to add the
try/finally.
- Around line 84-90: The centroid fallback mixes raw window_data in the
numerator with clipped weights in the denominator; change the numerator to use
the same clipped weights (i.e., compute weighted_avg =
float(numpy.sum(window_idx * weights) / numpy.sum(weights)) or window_idx *
clipped_weights) so both numerator and denominator use the same weights variable
(referencing weights, window_idx, peak_idx, weighted_avg) and preserve the
zero-sum check that sets weighted_avg = float(peak_idx) when weights.sum() == 0.
In `@src/odemis/acq/align/test/goffset_test.py`:
- Around line 25-26: Replace the hardcoded HOME_PATH and H5_FILE_2d_NO_PEAK
string construction with repository-relative pathlib resolution: use
Path(__file__).resolve().parent (or navigate from that directory) to build the
HDF5 fixture path and assign it to H5_FILE_2d_NO_PEAK as a Path object (or
convert to str where needed); update any code using HOME_PATH/H5_FILE_2d_NO_PEAK
to use the new Path-based value so tests no longer rely on ~/development/... and
follow the project guideline to prefer pathlib.
- Around line 552-558: The test currently swallows all exceptions from waiting
on the future returned by sparc_auto_grating_offset(self.spgr, self.detector) so
it doesn't verify cancellation; modify the test to explicitly cancel the future
(call f.cancel()), then assert the future reports cancellation (e.g.,
f.cancelled() or f.done()) and that calling f.result() raises
asyncio.CancelledError (use an assertion like assertRaises or context manager
for CancelledError) instead of a blanket try/except that swallows errors.
- Around line 44-50: Uncomment and re-enable the class-level cleanup method
tearDownClass so the spectrograph position is restored after the test suite:
implement/restore the previously commented tearDownClass that calls
cls.spgr.moveAbsSync(cls._original_position) inside a try/except and uses
logging.exception("Failed restoring spectrograph position") on failure; ensure
cls._original_position is captured during setUpClass and that cls.spgr is
available before attempting the move to avoid AttributeError.
- Around line 86-90: The test is treating the return of
estimate_goffset_scale(self.spgr, self.detector) as a float but the function
returns a tuple (scale, p0, p1); update the call to unpack the 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 names scale, p0, p1 when making the change.
- Line 337: The test uses an undefined attribute self.ccd causing
AttributeError; change the assertion in the test (the line asserting membership
of detector name in dets_for_first_grating) to reference the correctly
initialized attribute self.detector (i.e., use self.detector.name) so the test
uses the object created in setUpClass() instead of the nonexistent self.ccd.
- Around line 147-155: The test sets delta = 0 so no misalignment occurs; change
delta to a non-zero value (for example 1 or the goffset axis step like
self.spgr.axes["goffset"].step) so the subsequent spgr.moveRelSync({"goffset":
delta * direction}) actually moves the axis and creates the intended
misalignment; keep the existing bounds check using current, goffset_max and
direction to avoid moving out of range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25d1a706-7199-4b8e-b40e-36971f70c997
📒 Files selected for processing (3)
src/odemis/acq/align/goffset.pysrc/odemis/acq/align/test/goffset_test.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/cont/tabs/sparc2_align_tab.py
Comment on lines
+766
to
+778
| finally: | ||
| logging.info("Turning off brightlight") | ||
| try: | ||
| bl.power.value = (bl.power.range[0]) | ||
| except Exception: | ||
| logging.exception("Failed to turn off the light during alignment cleanup") | ||
|
|
||
| spectrograph.moveAbsSync(original_pos) | ||
| if selector: | ||
| selector.moveAbsSync(original_selector) | ||
|
|
||
| with future._task_lock: | ||
| future._task_state = FINISHED |
There was a problem hiding this comment.
Make the final hardware restore best-effort.
If either restore move fails here, the finally block raises a new exception, masks the original task outcome, and skips the FINISHED state update. The light shutdown above already handles cleanup this way; the position restores should do the same.
Possible fix
finally:
logging.info("Turning off brightlight")
try:
bl.power.value = (bl.power.range[0])
except Exception:
logging.exception("Failed to turn off the light during alignment cleanup")
- spectrograph.moveAbsSync(original_pos)
- if selector:
- selector.moveAbsSync(original_selector)
+ try:
+ spectrograph.moveAbsSync(original_pos)
+ except Exception:
+ logging.exception("Failed to restore spectrograph position")
+ if selector:
+ try:
+ selector.moveAbsSync(original_selector)
+ except Exception:
+ logging.exception("Failed to restore selector position")
with future._task_lock:
future._task_state = FINISHED🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/acq/align/goffset.py` around lines 766 - 778, The finally block
must make hardware restores best-effort: wrap the spectrograph.restore call
(spectrograph.moveAbsSync(original_pos)) in its own try/except that catches
Exception and calls logging.exception("Failed to restore spectrograph position
during alignment cleanup"); do the same for
selector.moveAbsSync(original_selector) only if selector is truthy, logging a
selector-specific message; ensure these try/excepts prevent exceptions from
propagating so the subsequent with future._task_lock: future._task_state =
FINISHED always executes (i.e., leave the FINISHED assignment outside those try
blocks or in its own guaranteed path).
Comment on lines
+279
to
+280
| f = auto_align_grating_detector_offsets(spectrograph=self.spgr, detectors=self.detector, selector=self.selector) | ||
| res = f.result(timeout=900) |
There was a problem hiding this comment.
These calls no longer match auto_align_grating_detector_offsets().
The helper now requires opm, align_mode, and bl, so each of these invocations raises TypeError before the test reaches its assertions.
Also applies to: 299-300, 321-322
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/acq/align/test/goffset_test.py` around lines 279 - 280, The test
calls to auto_align_grating_detector_offsets no longer match its signature;
update each invocation (e.g., the calls on lines referenced) to pass the
required arguments opm, align_mode, and bl — for example provide the test's
corresponding objects (e.g., self.opm, self.align_mode, self.bl or the proper
fixtures) along with spectrograph=self.spgr, detectors=self.detector,
selector=self.selector so the function is invoked with the new parameters.
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
4 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.