Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new "goffset" axis was added to the Shamrock driver, with global bounds for grating and detector offsets. The driver computes goffset as the sum of grating and detector offsets (GetGoffset) and exposes absolute and relative moves that route updates to GratingOffset or DetectorOffset depending on the active grating. Position reports now include goffset. The simulated Shamrock adds a delay proportional to offset changes. A YAML fixture was updated to replace the Andor camera with simcam.Camera, switch initialization to an image file, remove the power_supplier field, and add a spectrograph dependency. A test exercising the goffset axis was added. Sequence Diagram(s)sequenceDiagram
actor Client
participant ShamrockDriver as "Shamrock Driver"
participant Grating as "Grating Mechanism"
participant Detector as "Detector Mechanism"
Client->>ShamrockDriver: moveAbs(goffset, target_value)
activate ShamrockDriver
ShamrockDriver->>ShamrockDriver: _doSetGoffsetAbs(target_value)
ShamrockDriver->>ShamrockDriver: Read current grating index and offsets
ShamrockDriver->>ShamrockDriver: Compute current_grat_offset + current_det_offset
alt grating == 1
ShamrockDriver->>Grating: Set GratingOffset (if allowed)
activate Grating
Grating-->>ShamrockDriver: Ack
deactivate Grating
else grating > 1
ShamrockDriver->>Detector: Set DetectorOffset
activate Detector
Detector-->>ShamrockDriver: Ack
deactivate Detector
end
ShamrockDriver->>ShamrockDriver: _updatePosition (include goffset)
ShamrockDriver-->>Client: Return updated position
deactivate ShamrockDriver
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/odemis/driver/andorshrk.py (1)
505-506: Consider using an empty string forunitinstead ofNone.Other axes in this file use string units (e.g.,
"m","rad"). For consistency and to avoid potential issues with code that expects a string, consider usingunit=""for a unitless/dimensionless axis.Suggested change
- "goffset": model.Axis(unit=None, range=((GRAT_OFFSET_MIN+DET_OFFSET_MIN), (GRAT_OFFSET_MAX+DET_OFFSET_MAX))) + "goffset": model.Axis(unit="", range=(GRAT_OFFSET_MIN + DET_OFFSET_MIN, GRAT_OFFSET_MAX + DET_OFFSET_MAX))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorshrk.py` around lines 505 - 506, The "goffset" axis currently constructs model.Axis(unit=None, ...) which is inconsistent with other axes that use string units; change the unit argument to an empty string (unit="") in the model.Axis call for "goffset" so it uses a string-based, dimensionless unit like the other axes and avoids code that assumes unit is a string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install/linux/usr/share/odemis/sim/sparc2-focus-test.odm.yaml`:
- Line 101: The YAML declares a "spectrograph" dependency that
simcam.Camera.__init__ doesn't handle (Camera only checks "focus" and "mirror"),
so either remove the unused "spectrograph" key from the dependencies or update
simcam.Camera.__init__ to accept and use it (e.g., read
deps.get("spectrograph"), assign to self.spectrograph, and implement any needed
behavior or wiring), or alternatively add a clear comment in the YAML noting
it's intentionally unused for future use.
In `@src/odemis/driver/andorshrk.py`:
- Around line 1606-1619: Add a return type annotation and a one-line docstring
to GetGoffset: annotate it as returning float (def GetGoffset(self) -> float)
and add a docstring describing what it computes (e.g., "Return total grating
offset: grating offset plus detector offset, accounting for input/output flipper
positions.") and mention that it uses GetGrating(),
GetFlipperMirror(INPUT_FLIPPER)/GetFlipperMirror(OUTPUT_FLIPPER),
GetGratingOffset(), and GetDetectorOffset() to compute the value. Ensure the
docstring follows the project's style and sits immediately under the def line.
- Around line 2093-2130: Add type hints and docstrings to the two offset
methods: annotate _doSetGoffsetAbs(self, target_offset: int, *,
allow_grating_offset: bool = True) -> None with a short docstring describing
parameters, behavior (uses grating/detector offsets, locks _hw_access) and that
it updates internal position but returns nothing; annotate
_doSetGoffsetRel(self, shift: int) -> None with a docstring stating it reads
current "goffset" and delegates to _doSetGoffsetAbs to apply a relative shift,
and remove the unnecessary return keyword in _doSetGoffsetRel so it doesn't
return a value. Ensure the docstrings mention any side effects (calls
SetGratingOffset/SetDetectorOffset and _updatePosition) and document default for
allow_grating_offset.
- Around line 57-60: Remove the extraneous "#CHECK!!" comments appended to the
offset constants DET_OFFSET_MIN, DET_OFFSET_MAX, GRAT_OFFSET_MIN, and
GRAT_OFFSET_MAX in andorshrk.py; update the constant declarations so they remain
unchanged numerically but no longer include the "#CHECK!!" inline comments
(these values are validated by test_goffset()).
---
Nitpick comments:
In `@src/odemis/driver/andorshrk.py`:
- Around line 505-506: The "goffset" axis currently constructs
model.Axis(unit=None, ...) which is inconsistent with other axes that use string
units; change the unit argument to an empty string (unit="") in the model.Axis
call for "goffset" so it uses a string-based, dimensionless unit like the other
axes and avoids code that assumes unit is a string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a251f3a5-1211-4fb9-887a-3fe70cb475ea
📒 Files selected for processing (3)
install/linux/usr/share/odemis/sim/sparc2-focus-test.odm.yamlsrc/odemis/driver/andorshrk.pysrc/odemis/driver/test/andorshrk_test.py
| targetTemperature: -60, # °C | ||
| image: "sparc-spec-sim.h5", | ||
| }, | ||
| dependencies: {spectrograph: "Spectrograph"}, |
There was a problem hiding this comment.
The spectrograph dependency will be silently ignored.
Based on the simcam.Camera.__init__ implementation (src/odemis/driver/simcam.py:47-70 and lines 140-175), only "focus" and "mirror" dependency keys are handled. The "spectrograph" key specified here is not checked or used anywhere in the Camera class, so this dependency wiring has no effect at runtime.
If the dependency is intended for documentation or future use, consider adding a comment. Otherwise, either remove the unused dependency or update simcam.Camera to handle it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install/linux/usr/share/odemis/sim/sparc2-focus-test.odm.yaml` at line 101,
The YAML declares a "spectrograph" dependency that simcam.Camera.__init__
doesn't handle (Camera only checks "focus" and "mirror"), so either remove the
unused "spectrograph" key from the dependencies or update simcam.Camera.__init__
to accept and use it (e.g., read deps.get("spectrograph"), assign to
self.spectrograph, and implement any needed behavior or wiring), or
alternatively add a clear comment in the YAML noting it's intentionally unused
for future use.
Comment on lines
+2093
to
+2130
| def _doSetGoffsetAbs(self, target_offset, *, allow_grating_offset=True): | ||
| target_offset = int(round(target_offset)) # ensure that we get integers for steps | ||
| grating = self.GetGrating() | ||
|
|
||
| if "flip-in" in self.axes: | ||
| flip_in_pos = self.GetFlipperMirror(INPUT_FLIPPER) | ||
| else: | ||
| flip_in_pos = 0 | ||
|
|
||
| if "flip-out" in self.axes: | ||
| flip_out_pos = self.GetFlipperMirror(OUTPUT_FLIPPER) | ||
| else: | ||
| flip_out_pos = 0 | ||
|
|
||
| with self._hw_access: | ||
| current_grat_offset = self.GetGratingOffset(grating) | ||
| current_det_offset = self.GetDetectorOffset(flip_in_pos, flip_out_pos) | ||
| logging.debug("Current goffset: %d (Grat: %d, Det: %d)", | ||
| (current_grat_offset + current_det_offset), | ||
| current_grat_offset, current_det_offset) | ||
|
|
||
| if grating == 1: | ||
| if not allow_grating_offset: | ||
| logging.debug("Grating offset update disabled (grating=1, target=%d)",target_offset,) | ||
| else: | ||
| grating_offset = target_offset - current_det_offset | ||
| self.SetGratingOffset(grating, grating_offset) | ||
|
|
||
| elif grating > 1: | ||
| detector_offset = target_offset - current_grat_offset | ||
| self.SetDetectorOffset(flip_in_pos, flip_out_pos, detector_offset) | ||
|
|
||
| self._updatePosition() | ||
|
|
||
| def _doSetGoffsetRel(self, shift): | ||
| current_pos = self.position.value.get("goffset", 0) | ||
| return self._doSetGoffsetAbs(current_pos + shift) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and docstrings for the new offset methods.
Both _doSetGoffsetAbs and _doSetGoffsetRel are missing type annotations and docstrings as required by coding guidelines.
Proposed fix
- def _doSetGoffsetAbs(self, target_offset, *, allow_grating_offset=True):
- target_offset = int(round(target_offset)) # ensure that we get integers for steps
+ def _doSetGoffsetAbs(self, target_offset: float, *, allow_grating_offset: bool = True) -> None:
+ """
+ Change the global offset (goffset) to an absolute value.
+
+ For grating 1, adjusts the grating offset. For other gratings, adjusts the
+ detector offset to achieve the target combined offset.
+
+ :param target_offset: target combined offset value in steps
+ :param allow_grating_offset: if False and grating is 1, skip updating the grating offset
+ """
+ target_offset = int(round(target_offset)) # ensure that we get integers for steps
grating = self.GetGrating()- def _doSetGoffsetRel(self, shift):
- current_pos = self.position.value.get("goffset", 0)
- return self._doSetGoffsetAbs(current_pos + shift)
+ def _doSetGoffsetRel(self, shift: float) -> None:
+ """
+ Change the global offset (goffset) by a relative value.
+
+ :param shift: relative offset change in steps
+ """
+ current_pos = self.position.value.get("goffset", 0)
+ self._doSetGoffsetAbs(current_pos + shift)Note: Also removed unnecessary return in _doSetGoffsetRel since the method doesn't return a meaningful value.
As per coding guidelines: "Always use type hints for function parameters and return types" and "Include docstrings for all functions and classes".
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 2094-2094: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/andorshrk.py` around lines 2093 - 2130, Add type hints and
docstrings to the two offset methods: annotate _doSetGoffsetAbs(self,
target_offset: int, *, allow_grating_offset: bool = True) -> None with a short
docstring describing parameters, behavior (uses grating/detector offsets, locks
_hw_access) and that it updates internal position but returns nothing; annotate
_doSetGoffsetRel(self, shift: int) -> None with a docstring stating it reads
current "goffset" and delegates to _doSetGoffsetAbs to apply a relative shift,
and remove the unnecessary return keyword in _doSetGoffsetRel so it doesn't
return a value. Ensure the docstrings mention any side effects (calls
SetGratingOffset/SetDetectorOffset and _updatePosition) and document default for
allow_grating_offset.
pieleric
requested changes
Mar 6, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/odemis/driver/andorshrk.py (1)
2130-2130: Minor formatting: add space around+in the debug message.Proposed fix
- logging.debug("Grating offset update disabled (grating=1, target=%d)",target_offset,) + logging.debug("Grating offset update disabled (grating=1, target=%d)", target_offset)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorshrk.py` at line 2130, The debug log call using logging.debug that prints the grating offset (the line with logging.debug("Grating offset update disabled (grating=1, target=%d)", target_offset,)) should be reformatted to ensure spaces surround any + operators in the message; update the logging.debug invocation that references target_offset so concatenation (if used) has spaces around '+' or, better, use a formatted string/percent-format with proper spacing in the message text to include spaces around any '+' and keep the target_offset interpolation unchanged.
🤖 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/driver/andorshrk.py`:
- Around line 2135-2137: Replace the invalid "else grating > 1:" line with a
proper unconditional else: because the prior branch handles grating == 1 and
grating values start at 1, change the clause to "else:" so the block that
computes detector_offset and calls self.SetDetectorOffset(flip_in_pos,
flip_out_pos, detector_offset) executes for all non-1 grating cases; ensure the
surrounding indentation and use of target_offset/current_grat_offset remain
unchanged.
---
Nitpick comments:
In `@src/odemis/driver/andorshrk.py`:
- Line 2130: The debug log call using logging.debug that prints the grating
offset (the line with logging.debug("Grating offset update disabled (grating=1,
target=%d)", target_offset,)) should be reformatted to ensure spaces surround
any + operators in the message; update the logging.debug invocation that
references target_offset so concatenation (if used) has spaces around '+' or,
better, use a formatted string/percent-format with proper spacing in the message
text to include spaces around any '+' and keep the target_offset interpolation
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6df33f40-1299-46cb-9e13-c0e23c0b8ba4
📒 Files selected for processing (1)
src/odemis/driver/andorshrk.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/odemis/driver/andorshrk.py (1)
53-56: Consider adding an explanatory comment for the offset constants.Per the past review discussion where Andor confirmed these values, it would be helpful to document why these bounds are intentionally much larger than typical calibration offsets:
📝 Suggested comment
+# Detector and grating offset limits (confirmed by Andor; values are intentionally +# much larger than typical calibration offsets and should rarely be reached in practice) SHAMROCK_DET_OFFSET_MIN = -240000 SHAMROCK_DET_OFFSET_MAX = 240000 SHAMROCK_GRAT_OFFSET_MIN = -20000 SHAMROCK_GRAT_OFFSET_MAX = 20000Based on learnings: these offset constants have been officially confirmed as correct by Andor for all Shamrock/Kymera spectrographs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorshrk.py` around lines 53 - 56, Add a concise explanatory comment above the SHAMROCK_DET_OFFSET_MIN / SHAMROCK_DET_OFFSET_MAX and SHAMROCK_GRAT_OFFSET_MIN / SHAMROCK_GRAT_OFFSET_MAX constants explaining that these unusually large bounds were confirmed by Andor for all Shamrock/Kymera spectrographs, are intentionally larger than typical calibration offsets to accommodate vendor-recommended hardware/firmware limits and valid remote/configured offsets, and should not be reduced without vendor re-confirmation; reference the constant names in the comment so future readers know which bounds are covered.
🤖 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/driver/andorshrk.py`:
- Around line 2124-2133: Compute grating_offset and detector_offset as currently
done, but before calling SetGratingOffset or SetDetectorOffset fetch the allowed
min/max for that offset (e.g. via an SDK/driver method such as a
Grating/Detector offset-limits getter or known constants on self), verify min <=
offset <= max, and if out of range raise ValueError with a clear message
including the offset, allowed range, grating/detector identifiers (for example:
raise ValueError(f"grating_offset {grating_offset} out of range [{min},{max}]
for grating {grating}")), otherwise call self.SetGratingOffset(grating,
grating_offset) or self.SetDetectorOffset(flip_in_pos, flip_out_pos,
detector_offset) as before.
---
Nitpick comments:
In `@src/odemis/driver/andorshrk.py`:
- Around line 53-56: Add a concise explanatory comment above the
SHAMROCK_DET_OFFSET_MIN / SHAMROCK_DET_OFFSET_MAX and SHAMROCK_GRAT_OFFSET_MIN /
SHAMROCK_GRAT_OFFSET_MAX constants explaining that these unusually large bounds
were confirmed by Andor for all Shamrock/Kymera spectrographs, are intentionally
larger than typical calibration offsets to accommodate vendor-recommended
hardware/firmware limits and valid remote/configured offsets, and should not be
reduced without vendor re-confirmation; reference the constant names in the
comment so future readers know which bounds are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f528a8a-5a79-459c-a53c-40a2fce83038
📒 Files selected for processing (1)
src/odemis/driver/andorshrk.py
pieleric
requested changes
Mar 10, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/odemis/driver/andorshrk.py (2)
1603-1626: Add return type annotation.Per coding guidelines, all functions require type hints for return types.
♻️ Proposed fix
- def GetGoffset(self): + def GetGoffset(self) -> int: """ Checks the current grating and flip-mirror positions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorshrk.py` around lines 1603 - 1626, The method GetGoffset lacks a return type annotation; update its signature (def GetGoffset(self)) to include the appropriate return type (e.g., -> float) to match the values returned by GetGratingOffset and GetDetectorOffset, so change the signature for GetGoffset to include the return type and keep the implementation unchanged.
2137-2140: Add type hints, docstring, and remove unnecessaryreturn.Per coding guidelines, type hints and docstrings are required. Also, since
_doSetGoffsetAbsdoesn't return a meaningful value, thereturnkeyword is unnecessary.♻️ Proposed fix
- def _doSetGoffsetRel(self, shift): - # We expect the goffset axis to exist - current_pos = self.position.value["goffset"] - return self._doSetGoffsetAbs(current_pos + shift) + def _doSetGoffsetRel(self, shift: float) -> None: + """ + Change the global offset (goffset) by a relative value. + + :param shift: relative offset change in steps + """ + current_pos = self.position.value["goffset"] + self._doSetGoffsetAbs(current_pos + shift)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorshrk.py` around lines 2137 - 2140, Add a concise docstring and type hints to the _doSetGoffsetRel method and remove the unnecessary return; specifically, change def _doSetGoffsetRel(self, shift): to include a type for shift (e.g., float) and a return type of None, add a one-line docstring explaining it sets the goffset axis relative to current position, compute current_pos = self.position.value["goffset"], call self._doSetGoffsetAbs(current_pos + shift) without returning its result (since _doSetGoffsetAbs does not return a meaningful value), and ensure references to _doSetGoffsetAbs and position are 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/driver/andorshrk.py`:
- Around line 2100-2135: Add a concise docstring and type hints to
_doSetGoffsetAbs(self, target_offset: float, *, allow_grating_offset: bool =
True) -> None; remove the redundant int(round(...)) by converting with round()
-> int only once (e.g., target = int(round(target_offset))). Before calling
SetGratingOffset or SetDetectorOffset, validate the computed grating_offset and
detector_offset against the SDK/driver limits (query whatever API on this class
provides such limits, e.g. a GetGratingOffsetRange/GetDetectorOffsetRange method
or attributes like grating_offset_min/max) and either clamp to the allowed range
or raise a clear ValueError with the attempted and allowed values; update
logging to include the final applied offset and whether clamping occurred, and
keep the existing calls to GetGrating, GetFlipperMirror, GetGratingOffset,
GetDetectorOffset, SetGratingOffset, SetDetectorOffset, and _updatePosition
unchanged otherwise.
---
Nitpick comments:
In `@src/odemis/driver/andorshrk.py`:
- Around line 1603-1626: The method GetGoffset lacks a return type annotation;
update its signature (def GetGoffset(self)) to include the appropriate return
type (e.g., -> float) to match the values returned by GetGratingOffset and
GetDetectorOffset, so change the signature for GetGoffset to include the return
type and keep the implementation unchanged.
- Around line 2137-2140: Add a concise docstring and type hints to the
_doSetGoffsetRel method and remove the unnecessary return; specifically, change
def _doSetGoffsetRel(self, shift): to include a type for shift (e.g., float) and
a return type of None, add a one-line docstring explaining it sets the goffset
axis relative to current position, compute current_pos =
self.position.value["goffset"], call self._doSetGoffsetAbs(current_pos + shift)
without returning its result (since _doSetGoffsetAbs does not return a
meaningful value), and ensure references to _doSetGoffsetAbs and position are
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e16e1dba-31b8-48d0-ac12-c77bea2f1787
📒 Files selected for processing (1)
src/odemis/driver/andorshrk.py
yxkdejong
force-pushed
the
add-goffset-andorshrk
branch
from
8dab697 to
d4b6452
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/andorshrk.py (1)
1841-1847:⚠️ Potential issue | 🟠 MajorGive
goffseta deterministic place in multi-axis move ordering.
_doSetGoffsetAbs()depends on the current grating and flipper ports, and grating-offset changes can affect the wavelength mapping too. Right nowmoveRel()keeps caller dict order, andmoveAbs()only special-casesgrating/wavelength, so requests that combinegoffsetwithflip-in,flip-out, orwavelengthcan finish in the wrong final state depending on key order.Suggested ordering fix
- for axis, s in shift.items(): # order doesn't matter + for axis in util.sorted_according_to(shift.keys(), ("goffset", "wavelength")): + s = shift[axis] if axis == "wavelength": actions.append((axis, self._doSetWavelengthRel, s)) @@ - ordered_axes = util.sorted_according_to(pos.keys(), ("grating", "wavelength")) + ordered_axes = util.sorted_according_to( + pos.keys(), + ("grating", "flip-in", "flip-out", "goffset", "wavelength"), + )Also applies to: 1872-1887
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorshrk.py` around lines 1841 - 1847, The move ordering is non-deterministic for combined axes and must ensure goffset runs in a fixed, correct place: change moveRel (where actions are built from shift) and the similar block in moveAbs (around where grating/wavelength are special-cased) to not rely on dict order but to assemble actions by iterating a priority list like ['flip-in','flip-out','grating','goffset','wavelength','focus'] (or equivalent axis names used in the code) and append corresponding handlers (_doSetGoffsetRel/_doSetGoffsetAbs, _doSetWavelengthRel/_Abs, grating/flip handlers) in that explicit order so grating/flip are applied before goffset and wavelength mapping is updated after goffset; ensure you update both the moveRel and moveAbs action-construction sites mentioned and preserve existing handler tuples.
♻️ Duplicate comments (1)
src/odemis/driver/andorshrk.py (1)
2131-2140:⚠️ Potential issue | 🟠 MajorValidate the derived sub-offset before calling the SDK.
target_offsetis only checked against the combinedgoffsetrange. For many current states,target_offset - current_det_offsetortarget_offset - current_grat_offsetcan still land outside the real grating/detector limits, so this path will fall through to an opaque SDK error instead of failing with a clearValueError.Suggested validation
if grating == 1: if not allow_grating_offset: logging.debug("Grating offset update disabled (grating=1, target=%d)",target_offset,) else: grating_offset = target_offset - current_det_offset + if not GRAT_OFFSET_MIN <= grating_offset <= GRAT_OFFSET_MAX: + raise ValueError( + f"Grating offset {grating_offset} out of bounds " + f"[{GRAT_OFFSET_MIN}, {GRAT_OFFSET_MAX}]" + ) self.SetGratingOffset(grating, grating_offset) else: detector_offset = target_offset - current_grat_offset + if not DET_OFFSET_MIN <= detector_offset <= DET_OFFSET_MAX: + raise ValueError( + f"Detector offset {detector_offset} out of bounds " + f"[{DET_OFFSET_MIN}, {DET_OFFSET_MAX}]" + ) self.SetDetectorOffset(flip_in_pos, flip_out_pos, detector_offset)Based on learnings:
DET_OFFSET_MIN = -240000, DET_OFFSET_MAX = 240000, GRAT_OFFSET_MIN = -20000, GRAT_OFFSET_MAX = 20000have been officially confirmed as correct by Andor for Shamrock/Kymera spectrographs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorshrk.py` around lines 2131 - 2140, The code computes grating_offset = target_offset - current_det_offset and detector_offset = target_offset - current_grat_offset but never validates those derived values before calling the SDK; update the logic in the block that calls SetGratingOffset and SetDetectorOffset to check the computed grating_offset against GRAT_OFFSET_MIN/GRAT_OFFSET_MAX and detector_offset against DET_OFFSET_MIN/DET_OFFSET_MAX and raise a clear ValueError if out of range (include grating, target_offset, current_det_offset/current_grat_offset and the invalid offset in the message); only call SetGratingOffset(self.SetGratingOffset) or SetDetectorOffset(self.SetDetectorOffset) when the validated sub-offset is within its respective limits, and preserve the existing allow_grating_offset, flip_in_pos and flip_out_pos checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/odemis/driver/andorshrk.py`:
- Around line 1841-1847: The move ordering is non-deterministic for combined
axes and must ensure goffset runs in a fixed, correct place: change moveRel
(where actions are built from shift) and the similar block in moveAbs (around
where grating/wavelength are special-cased) to not rely on dict order but to
assemble actions by iterating a priority list like
['flip-in','flip-out','grating','goffset','wavelength','focus'] (or equivalent
axis names used in the code) and append corresponding handlers
(_doSetGoffsetRel/_doSetGoffsetAbs, _doSetWavelengthRel/_Abs, grating/flip
handlers) in that explicit order so grating/flip are applied before goffset and
wavelength mapping is updated after goffset; ensure you update both the moveRel
and moveAbs action-construction sites mentioned and preserve existing handler
tuples.
---
Duplicate comments:
In `@src/odemis/driver/andorshrk.py`:
- Around line 2131-2140: The code computes grating_offset = target_offset -
current_det_offset and detector_offset = target_offset - current_grat_offset but
never validates those derived values before calling the SDK; update the logic in
the block that calls SetGratingOffset and SetDetectorOffset to check the
computed grating_offset against GRAT_OFFSET_MIN/GRAT_OFFSET_MAX and
detector_offset against DET_OFFSET_MIN/DET_OFFSET_MAX and raise a clear
ValueError if out of range (include grating, target_offset,
current_det_offset/current_grat_offset and the invalid offset in the message);
only call SetGratingOffset(self.SetGratingOffset) or
SetDetectorOffset(self.SetDetectorOffset) when the validated sub-offset is
within its respective limits, and preserve the existing allow_grating_offset,
flip_in_pos and flip_out_pos checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cd06cac-6aaf-4758-8335-d5f39304651e
📒 Files selected for processing (1)
src/odemis/driver/andorshrk.py
yxkdejong
force-pushed
the
add-goffset-andorshrk
branch
6 times, most recently
from
4d08ada to
9b467cc
Compare
pieleric
requested changes
Mar 25, 2026
Comment on lines
+2106
to
+2108
| :param allow_grating_offset (bool): check to allow changing the grating offset, if false, only change | ||
| detector offset. | ||
| :param single_detector_mode (bool): if true, it will always change the grating offset, |
Member
There was a problem hiding this comment.
I'm confused about what these arguments are about, as they are no callers using them. Was it for testing, and now they can be removed?
I do understand that we need to handle the special case where only one detector is used, and it's on the second output port, then the grating offset should be used to adjust. That said, the good news is that looking at our system config database, it seems no SPARC has such setup, so we don't risk breaking too many systems yet. In general, actually, I think that the detector offset should be used for all the combinations of input and output port different from DIRECT/DIRECT.... but that gets even more tricky to just say if "DIRECT/DIRECT use grating offset otherwise use detector offset" especially because we have a few systems where the only input port is the SIDE one!
One alternative would be to change the rule to say: "if grating > 1 or input,output == DIRECT,DIRECT", then use the grating offset. In such way, if there is only a SIDE detector, it'd still be possible to adjust the grating offset. However, this would require that the alignment procedure is extra aware of it, and always starts with the first grating and first detector, and then switch to the other detectors, to calibrate all the detector offset, and then moves on with the other gratings.
yxkdejong
force-pushed
the
add-goffset-andorshrk
branch
from
9b467cc to
ca5b87b
Compare
tepals
requested changes
Mar 30, 2026
| def ShamrockSetGratingOffset(self, device, grating, offset): | ||
| cur_offset = self._goffset[_val(grating) - 1] | ||
| new_offset = _val(offset) | ||
| time.sleep(abs(cur_offset - new_offset) / 10000) |
Contributor
There was a problem hiding this comment.
Can you add a comment to explain why the sleep is needed, and why you need to sleep the specified amount of time
yxkdejong
force-pushed
the
add-goffset-andorshrk
branch
2 times, most recently
from
4587863 to
3e3c643
Compare
yxkdejong
force-pushed
the
add-goffset-andorshrk
branch
from
3e3c643 to
15acde2
Compare
tepals
approved these changes
Apr 21, 2026
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
3 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.