[fix] PostureManager.get_posture_orientation() could return None#3376
[fix] PostureManager.get_posture_orientation() could return None#3376pieleric wants to merge 1 commit intodelmic:masterfrom
Conversation
It looks like it would always return a dict (position)... but passing a unsupported posture would cause it to return None implicitly. Instead, raise an exception in such case, to catch such error directly.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the PostureManager.get_posture_orientation() method where passing an unsupported posture value would cause the method to implicitly return None instead of raising an exception. The fix adds an explicit else clause that raises a KeyError with a descriptive error message when an unsupported posture is provided.
Changes:
- Added explicit exception handling for unsupported posture values in
get_posture_orientation()to catch errors directly instead of returning None implicitly
| column_tilt=self.fib_column_tilt) | ||
| return {"rx": rx, "rz": md["rz"]} | ||
| else: | ||
| raise KeyError(f"posture {POSITION_NAMES.get(posture, posture)} not supported for orientation retrieval") |
There was a problem hiding this comment.
ValueError should be used instead of KeyError for invalid parameter values. Throughout this codebase, ValueError is consistently used when an unsupported posture or target position parameter is provided (see lines 775, 778, 921, 949, 1063, etc.). KeyError is typically reserved for missing dictionary keys or axes (see lines 892, 1193, 1369). Since 'posture' is a method parameter that has an invalid value, ValueError is the more appropriate exception type.
| raise KeyError(f"posture {POSITION_NAMES.get(posture, posture)} not supported for orientation retrieval") | |
| raise ValueError(f"posture {POSITION_NAMES.get(posture, posture)} not supported for orientation retrieval") |
📝 WalkthroughWalkthroughThe change adds explicit error handling in the 🚥 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: 1
🤖 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 518-519: Replace the incorrect KeyError raised in the
orientation-retrieval branch with a ValueError so invalid posture argument
values use the canonical exception (change the raise in the else that currently
does raise KeyError(f"posture {POSITION_NAMES.get(posture, posture)} not
supported for orientation retrieval") to raise ValueError with the same
message), and update this function's/docstring to list ValueError as a possible
exception (keeping behavior consistent with other methods like to_posture(),
getTargetPosition(), and _doCryoSwitchSamplePosition()).
| else: | ||
| raise KeyError(f"posture {POSITION_NAMES.get(posture, posture)} not supported for orientation retrieval") |
There was a problem hiding this comment.
Use ValueError instead of KeyError for an unsupported argument value.
KeyError is semantically reserved for missing dict/mapping keys. Here, posture is an integer argument with an invalid value — the canonical exception for that is ValueError, which is also what every other unsupported-posture path in this file raises (to_posture(), getTargetPosition(), _doCryoSwitchSamplePosition() in all subclasses, etc.).
Using KeyError also risks being silently swallowed: each _doCryoSwitchSamplePosition implementation contains a narrowly scoped except KeyError block that converts a POSITION_NAMES[target] lookup failure into a ValueError. If the call chain happens to cross one of those boundaries, the wrong error message is surfaced.
Additionally, the docstring should be updated to document the newly raised exception.
🛠 Proposed fix
- else:
- raise KeyError(f"posture {POSITION_NAMES.get(posture, posture)} not supported for orientation retrieval")
+ else:
+ raise ValueError(f"posture {POSITION_NAMES.get(posture, posture)} not supported for orientation retrieval")And update the docstring:
def get_posture_orientation(self, posture: int) -> Dict[str, float]:
"""Get the orientation of the stage for the given posture
:param posture: the posture to get the orientation for
- :return: a dict with the orientation of the stage for the given posture"""
+ :return: a dict with the orientation of the stage for the given posture
+ :raises ValueError: if the posture is not supported for orientation retrieval
+ """🧰 Tools
🪛 Ruff (0.15.1)
[warning] 519-519: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 518 - 519, Replace the incorrect
KeyError raised in the orientation-retrieval branch with a ValueError so invalid
posture argument values use the canonical exception (change the raise in the
else that currently does raise KeyError(f"posture {POSITION_NAMES.get(posture,
posture)} not supported for orientation retrieval") to raise ValueError with the
same message), and update this function's/docstring to list ValueError as a
possible exception (keeping behavior consistent with other methods like
to_posture(), getTargetPosition(), and _doCryoSwitchSamplePosition()).
1 participant
It looks like it would always return a dict (position)... but passing a
unsupported posture would cause it to return None implicitly.
Instead, raise an exception in such case, to catch such error directly.