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.
| def GetGoffset(self): | ||
| 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 | ||
|
|
||
| goffset = self.GetGratingOffset(grating) + self.GetDetectorOffset(flip_in_pos, flip_out_pos) | ||
| return goffset |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hint and docstring per coding guidelines.
The method is missing the return type annotation and a docstring as required by the repository's coding guidelines.
Proposed fix
- def GetGoffset(self):
+ def GetGoffset(self) -> int:
+ """
+ Computes the current global offset (goffset), which is the sum of the
+ grating offset for the current grating and the detector offset for the
+ current flipper positions.
+
+ :return: the combined offset value in steps
+ """
grating = self.GetGrating()As per coding guidelines: "Always use type hints for function parameters and return types" and "Include docstrings for all functions and classes".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def GetGoffset(self): | |
| 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 | |
| goffset = self.GetGratingOffset(grating) + self.GetDetectorOffset(flip_in_pos, flip_out_pos) | |
| return goffset | |
| def GetGoffset(self) -> int: | |
| """ | |
| Computes the current global offset (goffset), which is the sum of the | |
| grating offset for the current grating and the detector offset for the | |
| current flipper positions. | |
| :return: the combined offset value in 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 | |
| goffset = self.GetGratingOffset(grating) + self.GetDetectorOffset(flip_in_pos, flip_out_pos) | |
| return goffset |
🤖 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 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.
| 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.
src/odemis/driver/andorshrk.py
Outdated
| if "flip-in" in self.axes: | ||
| flip_in_pos = self.GetFlipperMirror(INPUT_FLIPPER) | ||
| else: | ||
| flip_in_pos = 0 |
There was a problem hiding this comment.
Let's replace the 0 by the more explicit DIRECT_PORT. Please do it everywhere with the similar pattern.
| else: | ||
| flip_out_pos = 0 | ||
|
|
||
| goffset = self.GetGratingOffset(grating) + self.GetDetectorOffset(flip_in_pos, flip_out_pos) |
There was a problem hiding this comment.
We need to have a (potentially long) comment explaining the "smartness" (aka "trick") about reporting the grating offset as grating offset + detector offset.
src/odemis/driver/andorshrk.py
Outdated
| else: | ||
| flip_out_pos = 0 | ||
|
|
||
| with self._hw_access: |
There was a problem hiding this comment.
All the functions GetOffset() and SetOffset() already take the lock, so I don't think you need to hold it here... unless you want to be very sure that no other function call would happen in between the function calls of this block. I don't think it's a requirement.
src/odemis/driver/andorshrk.py
Outdated
| (current_grat_offset + current_det_offset), | ||
| current_grat_offset, current_det_offset) | ||
|
|
||
| if grating == 1: |
There was a problem hiding this comment.
Please add a comment explain (again) the hack of offset = grating offset + detector offset.
src/odemis/driver/andorshrk.py
Outdated
| self._updatePosition() | ||
|
|
||
| def _doSetGoffsetRel(self, shift): | ||
| current_pos = self.position.value.get("goffset", 0) |
There was a problem hiding this comment.
If this function is called while the "goffset" axis doesn't exist, something went very wrong. So it should be safe to just write:
current_pos = self.position.value["goffset"]
src/odemis/driver/andorshrk.py
Outdated
| grating_offset = target_offset - current_det_offset | ||
| self.SetGratingOffset(grating, grating_offset) | ||
|
|
||
| elif grating > 1: |
There was a problem hiding this comment.
Just else. Then you're certain to handle every cases.
src/odemis/driver/andorshrk.py
Outdated
|
|
||
| elif grating > 1: | ||
| detector_offset = target_offset - current_grat_offset | ||
| self.SetDetectorOffset(flip_in_pos, flip_out_pos, detector_offset) |
There was a problem hiding this comment.
Did you check what sort of error happen if you request a value out of bound? As the offset is composed of 2 different offsets, the ranges can end up outside of the accepted bound. In this case, it'd be best to raise a clear ValueError with a message explaining the "sub" offset is out of bound (with the actual value).
Probably the simplest is to check the bounds before even calling Set*Offset().
| # restore the original offset so other tests aren't affected | ||
| sp.moveAbsSync({"goffset": orig_pos}) |
There was a problem hiding this comment.
This is incorrect. There is one offset per grating and detector. So you'd have to store the offset per grating and detector, and restore them all here.
src/odemis/driver/andorshrk.py
Outdated
| speed=(max_speed, max_speed)), | ||
| "grating": model.Axis(choices=gchoices) | ||
| "grating": model.Axis(choices=gchoices), | ||
| "goffset": model.Axis(unit=None, range=((GRAT_OFFSET_MIN+DET_OFFSET_MIN), (GRAT_OFFSET_MAX+DET_OFFSET_MAX))) |
There was a problem hiding this comment.
Please adjust the formatting. Make it 2 lines, and with spaces around +.
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
src/odemis/driver/andorshrk.py
Outdated
| SHAMROCK_DET_OFFSET_MIN = -240000 | ||
| SHAMROCK_DET_OFFSET_MAX = 240000 | ||
| SHAMROCK_GRAT_OFFSET_MIN = -20000 | ||
| SHAMROCK_GRAT_OFFSET_MAX = 20000 |
There was a problem hiding this comment.
Please drop the SHAMROCK_ prefix. That's only helpful in C.
| The detector offset is the equivalent grating offset that is needed to compensate for the change in | ||
| optical path length, introduced by the flip-mirrors. |
There was a problem hiding this comment.
Technically, I don't know if that's correct. At least, in my head, I had in mind that the "offset" is not to compensate the path length, but more the (slightly imprecise) angle of the mirrors (the flipper mirrors in particular, for the detector offset). That said, I've never read any actual statement about what it corrects so I might be completely wrong! Can you confirm what is compensated?
There was a problem hiding this comment.
To be honest, I also don't fully know what it corrects, because I just copied this part from the project guidelines... Where can I check this? Or should I ask Noémie maybe?
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
| 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 = DIRECT_PORT | ||
|
|
||
| if "flip-out" in self.axes: | ||
| flip_out_pos = self.GetFlipperMirror(OUTPUT_FLIPPER) | ||
| else: | ||
| flip_out_pos = DIRECT_PORT | ||
|
|
||
| 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) | ||
|
|
||
| # The effective grating offset is the actual grating offset + the detector offset to account for changes in the detector path | ||
| # (e.g. flipper mirrors) that shift the spectrum on the detector. By adding the detector offset to the grating offset, | ||
| # the reported goffset always matches the observed spectral alignment, keeping calibration consistent. | ||
|
|
||
| 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) | ||
|
|
||
| else: | ||
| detector_offset = target_offset - current_grat_offset | ||
| self.SetDetectorOffset(flip_in_pos, flip_out_pos, detector_offset) | ||
|
|
||
| self._updatePosition() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints, docstring, and bounds validation.
This method has several issues that should be addressed:
- Missing type hints and docstring per coding guidelines.
- Line 2101:
round()already returns anintin Python 3, making theint()call redundant. - No bounds validation before calling
SetGratingOffsetorSetDetectorOffset- if the computed offset exceeds the allowed range, the SDK will raise an unclear error.
♻️ 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 = 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 = DIRECT_PORT
if "flip-out" in self.axes:
flip_out_pos = self.GetFlipperMirror(OUTPUT_FLIPPER)
else:
flip_out_pos = DIRECT_PORT
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)
# The effective grating offset is the actual grating offset + the detector offset to account for changes in the detector path
# (e.g. flipper mirrors) that shift the spectrum on the detector. By adding the detector offset to the grating offset,
# the reported goffset always matches the observed spectral alignment, keeping calibration consistent.
if grating == 1:
if not allow_grating_offset:
- logging.debug("Grating offset update disabled (grating=1, target=%d)",target_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)
self._updatePosition()🧰 Tools
🪛 Ruff (0.15.5)
[warning] 2101-2101: 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 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.
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
from
d4b6452 to
64282a8
Compare
yxkdejong
force-pushed
the
add-goffset-andorshrk
branch
from
64282a8 to
208f646
Compare
2 participants
No description provided.