[fix] MeteorPostureManager: only trust the scan rotation is identical on both beams#3382
Open
pieleric wants to merge 1 commit intodelmic:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors scanner rotation correction in the acquisition module by removing the rotation parameter from _set_scanner_rotation_cor and making it compute rotation from each scanner's current rotation.value (iterating "e-beam" and "ion-beam" and skipping missing scanners). Call sites were updated to call _set_scanner_rotation_cor() with no arguments. _get_scan_rotation was simplified to require both e-beam and ion-beam to be present to trust a scan rotation, and _update_conversion now calls the no-arg scanner correction before using _get_scan_rotation. Sequence Diagram(s)(omitted) Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
703-705: Consider shortening the inlineValueErrormessage (Ruff TRY003).Ruff flags the long exception message at line 705. Consider moving the message text into the
ValueErrorsubclass or shortening it, though this is a style preference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/move.py` around lines 703 - 705, The ValueError raised in the rotation check (where sr and ion_sr are compared using numpy.isclose with ATOL_ROTATION_POS in move.py) has an overly long inline f-string; shorten it to a brief message like "SEM and FIB rotations mismatch" or define a small custom exception (e.g., RotationMismatchError) and raise that instead of the long f-string, keeping the numeric details out of the exception message (you can log sr and ion_sr separately if needed); update the raise statement that currently reads raise ValueError(f"The SEM and FIB rotations do not match {sr} != {ion_sr}") to use the shorter message or the new exception class.
🤖 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/move.py`:
- Around line 709-722: The function _set_scanner_rotation_cor is missing an
explicit return type annotation; update its signature to declare a return type
of None (e.g., def _set_scanner_rotation_cor(self) -> None:) and adjust any
related type hints if present; locate the method by name and ensure other
references (model.getComponent, scanner.rotation, scanner.updateMetadata,
model.MD_ROTATION_COR) remain unchanged.
- Around line 683-707: Update the _get_scan_rotation docstring to include a
:raises: entry documenting that a ValueError is raised when both the e-beam and
ion-beam components exist but their scan rotation values differ (i.e., when
numpy.isclose(sr, ion_sr, atol=ATOL_ROTATION_POS) is false); keep the same short
description style used elsewhere and reference the ValueError and the condition
(mismatched SEM/FIB rotations) in the :raises: line.
---
Nitpick comments:
In `@src/odemis/acq/move.py`:
- Around line 703-705: The ValueError raised in the rotation check (where sr and
ion_sr are compared using numpy.isclose with ATOL_ROTATION_POS in move.py) has
an overly long inline f-string; shorten it to a brief message like "SEM and FIB
rotations mismatch" or define a small custom exception (e.g.,
RotationMismatchError) and raise that instead of the long f-string, keeping the
numeric details out of the exception message (you can log sr and ion_sr
separately if needed); update the raise statement that currently reads raise
ValueError(f"The SEM and FIB rotations do not match {sr} != {ion_sr}") to use
the shorter message or the new exception class.
Comment on lines
+709
to
+722
| def _set_scanner_rotation_cor(self): | ||
| """ | ||
| Set the scanners' MD_ROTATION_COR metadata field to the provided rotation value. | ||
| :param rotation: rotation in radians | ||
| Set the scanners' MD_ROTATION_COR metadata field to the current rotation, so that the | ||
| image is shown without rotation in the UI, as in the SEM UI. This is necessary, as | ||
| the sample stage compensates for the scan rotation already. | ||
| """ | ||
| for scanner_name in ["e-beam", "ion-beam"]: | ||
| scanner = None | ||
| try: | ||
| scanner = model.getComponent(role=scanner_name) | ||
| except LookupError: | ||
| pass | ||
| continue | ||
|
|
||
| if scanner is not None: | ||
| scanner.updateMetadata({model.MD_ROTATION_COR: rotation}) | ||
| rotation = scanner.rotation.value | ||
| scanner.updateMetadata({model.MD_ROTATION_COR: rotation}) |
There was a problem hiding this comment.
Missing -> None return type annotation on _set_scanner_rotation_cor.
Per coding guidelines, all functions must declare return types.
📝 Proposed fix
- def _set_scanner_rotation_cor(self):
+ def _set_scanner_rotation_cor(self) -> None:As per coding guidelines: "Always use type hints for function parameters and return types in Python code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/move.py` around lines 709 - 722, The function
_set_scanner_rotation_cor is missing an explicit return type annotation; update
its signature to declare a return type of None (e.g., def
_set_scanner_rotation_cor(self) -> None:) and adjust any related type hints if
present; locate the method by name and ensure other references
(model.getComponent, scanner.rotation, scanner.updateMetadata,
model.MD_ROTATION_COR) remain unchanged.
… on both beams Commit 42ecdd0 (PostureManager: adjust orientation of sample stage in SEM imaging posture) changed the behaviour with scan rotation. It introduced a default scan rotation to 180° on some systems (eg, TFS1), and also accepted the e-beam scan rotation as-is if no ion-beam is present. However, until now the TFS1 systems never cared about the e-beam scan rotation. So users don't pay attention to it. However, the FM movement assumes the scan rotation is 180°, always. So if TFS1 microscope file accessed the e-beam (some do, others don't), then the movement of the FM would depend on the e-beam scan rotation when starting Odemis. That shouldn't be. => Revert to only trust the scan rotation if both e- and ion-beam scna rotation is the same. If there is only an e-beam, use the default scan rotation. This should ensure that the stage behaviour stays the same as on previous version of Odemis, and only use the scan rotation to adjust the stage movement on FIB/SEM systems, where movement in FIB/SEM postures matters.
pieleric
force-pushed
the
fix-meteorposturemanager-only-trust-the-scan-rotation-is-identical-on-both-beams
branch
from
c55ef18 to
5383e7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/odemis/acq/move.py (1)
710-710:⚠️ Potential issue | 🟡 MinorMissing
-> Nonereturn type annotation on_set_scanner_rotation_cor.📝 Proposed fix
- def _set_scanner_rotation_cor(self): + def _set_scanner_rotation_cor(self) -> None:As per coding guidelines: "Always use type hints for function parameters and return types in Python code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/move.py` at line 710, The method _set_scanner_rotation_cor is missing a return type annotation; update its signature to include an explicit return type -> None (e.g., def _set_scanner_rotation_cor(self) -> None:) to comply with project type-hinting guidelines, and run typechecker/linters to ensure no other functions in the same class (or overrides of _set_scanner_rotation_cor) are missing return annotations.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
706-706: Consider moving longValueErrormessage into a dedicated exception class (Ruff TRY003).The static analyser flags this as a style issue; extracting the message to a named exception would silence it, but this is low priority.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/move.py` at line 706, The long inline ValueError message in the raise statement inside function/method where the SEM and FIB rotations are compared should be replaced by a dedicated exception class (e.g., RotationMismatchError) to satisfy the Ruff TRY003 style rule; create a new exception class (subclass ValueError or a project base exception) that accepts sr and ion_sr (or stores a formatted message), replace the raise ValueError(...) with raise RotationMismatchError(sr, ion_sr) (or raise RotationMismatchError from the original), and update any imports/usages and tests to catch or reference RotationMismatchError instead of the old ValueError.
🤖 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/move.py`:
- Line 688: Update the reStructuredText docstring entry that currently reads
":raise: ValueError if the scan rotation values for e-beam and ion-beam do not
match" to use the canonical field name ":raises:" instead of ":raise:"; locate
the docstring containing that exact sentence (in move.py) and replace ":raise:"
with ":raises:" so the docs properly render and conform to the rST style guide.
---
Duplicate comments:
In `@src/odemis/acq/move.py`:
- Line 710: The method _set_scanner_rotation_cor is missing a return type
annotation; update its signature to include an explicit return type -> None
(e.g., def _set_scanner_rotation_cor(self) -> None:) to comply with project
type-hinting guidelines, and run typechecker/linters to ensure no other
functions in the same class (or overrides of _set_scanner_rotation_cor) are
missing return annotations.
---
Nitpick comments:
In `@src/odemis/acq/move.py`:
- Line 706: The long inline ValueError message in the raise statement inside
function/method where the SEM and FIB rotations are compared should be replaced
by a dedicated exception class (e.g., RotationMismatchError) to satisfy the Ruff
TRY003 style rule; create a new exception class (subclass ValueError or a
project base exception) that accepts sr and ion_sr (or stores a formatted
message), replace the raise ValueError(...) with raise RotationMismatchError(sr,
ion_sr) (or raise RotationMismatchError from the original), and update any
imports/usages and tests to catch or reference RotationMismatchError instead of
the old ValueError.
| Get the scan rotation value for SEM/FIB, and ensure they match. | ||
| If not both e-beam and ion-beam are available, the default scan rotation is used. | ||
| :return: the scan rotation value in radians | ||
| :raise: ValueError if the scan rotation values for e-beam and ion-beam do not match |
There was a problem hiding this comment.
Fix :raise: → :raises: in docstring.
:raise: is not a recognised reStructuredText info-field; the canonical directive is :raises:.
📝 Proposed fix
- :raise: ValueError if the scan rotation values for e-beam and ion-beam do not match
+ :raises ValueError: if both e-beam and ion-beam are present but their scan rotations do not matchAs per coding guidelines: "Include docstrings for all functions and classes, following the reStructuredText style guide."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/move.py` at line 688, Update the reStructuredText docstring
entry that currently reads ":raise: ValueError if the scan rotation values for
e-beam and ion-beam do not match" to use the canonical field name ":raises:"
instead of ":raise:"; locate the docstring containing that exact sentence (in
move.py) and replace ":raise:" with ":raises:" so the docs properly render and
conform to the rST style guide.
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
1 participant
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.
Commit 42ecdd0 (PostureManager: adjust orientation of sample stage
in SEM imaging posture) changed the behaviour with scan rotation. It
introduced a default scan rotation to 180° on some systems (eg, TFS1),
and also accepted the e-beam scan rotation as-is if no ion-beam is
present.
However, until now the TFS1 systems never cared about the e-beam scan
rotation. So users don't pay attention to it. However, the FM movement
assumes the scan rotation is 180°, always. So if TFS1 microscope file
accessed the e-beam (some do, others don't), then the movement of the FM
would depend on the e-beam scan rotation when starting Odemis. That
shouldn't be.
=> Revert to only trust the scan rotation if both e- and ion-beam scna
rotation is the same. If there is only an e-beam, use the default scan
rotation. This should ensure that the stage behaviour stays the same as
on previous version of Odemis, and only use the scan rotation to adjust
the stage movement on FIB/SEM systems, where movement in FIB/SEM
postures matters.