refactor: Improve microscope.py code quality and API design #402
Conversation
- Fix bug: move_z_to() now correctly passes blocking parameter to stage - Fix bug: acquire_image() now raises RuntimeError instead of silently continuing on failure - Replace print() with proper logging in acquire_image() - Replace TypeVar placeholders with None for conditional imports - Remove unused imports (serial, move_z_axis_to_safety_position) - Remove unnecessary super().__init__() call - Simplify boolean expression in send_hardware_trigger() - Make wait_for_microcontroller private (_wait_for_microcontroller) - Add type hints to all public method parameters - Add docstrings to all public methods - Make close() resilient to errors during shutdown - Add None check for microcontroller in close() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR improves code quality, fixes bugs, and enhances API design in microscope.py. The changes include fixing a missing blocking parameter in move_z_to(), improving error handling in acquire_image(), removing unused imports, replacing TypeVar placeholders with None, and adding comprehensive type hints and docstrings to all public methods.
- Fixed bug where
move_z_to()wasn't passing theblockingparameter to the underlying stage method - Enhanced
acquire_image()to raiseRuntimeErrorinstead of silently returningNonewhen camera fails - Added type hints and docstrings to all public methods, and made
waitForMicrocontrollerprivate as_wait_for_microcontroller
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_x(self) -> float: | ||
| return self.stage.get_pos().x_mm | ||
|
|
||
| def get_y(self): | ||
| def get_y(self) -> float: | ||
| return self.stage.get_pos().y_mm | ||
|
|
||
| def get_z(self): | ||
| def get_z(self) -> float: | ||
| return self.stage.get_pos().z_mm |
There was a problem hiding this comment.
The getter methods get_x(), get_y(), and get_z() are missing docstrings. All other public methods in this PR have been documented, but these three simple getters lack documentation explaining what they return. Consider adding docstrings to maintain consistency, for example:
def get_x(self) -> float:
"""Get the current X position of the stage.
Returns:
Current X position in mm.
"""
return self.stage.get_pos().x_mmAddress PR review feedback - add missing docstrings to position getter methods for consistency with other public methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ObjectiveChanger2PosController = None | ||
|
|
||
| if control._def.RUN_FLUIDICS: | ||
| from control.fluidics import Fluidics | ||
| else: | ||
| Fluidics = TypeVar("Fluidics") | ||
| Fluidics = None | ||
|
|
||
| if control._def.ENABLE_NL5: | ||
| import control.NL5 as NL5 | ||
| else: | ||
| NL5 = TypeVar("NL5") | ||
| NL5 = None |
There was a problem hiding this comment.
The TypeVar to None replacement creates type checking issues. When these classes are set to None (feature disabled), type hints like Optional[ObjectiveChanger2PosController] become Optional[None], which is semantically incorrect. Consider using a string literal type or proper type stubs for conditional imports, or use TYPE_CHECKING to conditionally import these for type hints only.
There was a problem hiding this comment.
@Alpaca233 should it be TypeVar or None?
There was a problem hiding this comment.
this is what CC says
Analysis: This is a valid concern. When ObjectiveChanger2PosController = None, the type hint Optional[ObjectiveChanger2PosController] becomes Optional[None] which equals None - not useful for type checkers. However, the original TypeVar usage was also incorrect (TypeVar is for generics, not placeholders). A proper fix would use TYPE_CHECKING pattern.
software/control/microscope.py
Outdated
| if image is None: | ||
| print("self.camera.read_frame() returned None") | ||
| self._log.error("camera.read_frame() returned None") | ||
| raise RuntimeError("Failed to acquire image: camera.read_frame() returned None") |
There was a problem hiding this comment.
When an exception is raised because camera.read_frame() returns None, the illumination is not turned off if SOFTWARE trigger mode is being used. This could leave the illumination on indefinitely. Consider wrapping the frame reading logic in a try-finally block to ensure illumination is always turned off when using SOFTWARE trigger mode.
Wrap frame reading in try-finally to guarantee illumination is turned off even when an exception is raised (e.g., camera returns None). Previously, a failed acquisition could leave illumination on indefinitely. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
move_z_to()not passingblockingparameter;acquire_image()now raisesRuntimeErroron failure instead of silently continuingTypeVarplaceholders withNone, simplified boolean expressions, improved error handling inclose()_wait_for_microcontrollerprivateChanges
Bug Fixes
move_z_to()now correctly passesblockingparameter to underlying stage methodacquire_image()raisesRuntimeErrorwhen camera fails to return a frame (previously just printed and returnedNone)Code Quality
serial,move_z_axis_to_safety_position)TypeVarplaceholders withNonefor conditional imports (cleaner pattern)super().__init__()callTrue if x else Falsetox is not Noneprint()with properself._log.error()loggingAPI Design
waitForMicrocontroller→_wait_for_microcontroller(private, snake_case)close()resilient to errors (each component close is wrapped in try-except)Nonecheck for microcontroller inclose()Test plan
python -m pytest tests/control/test_microscope.py- all tests passblackformatter🤖 Generated with Claude Code