Abstract base classes #1140
Abstract base classes #1140
Conversation
Updated DAQBase, FilterWheelBase, and GalvoBase to inherit from abc.ABC and define required abstract methods. Enforces implementation of key interface methods in subclasses and clarifies the intended usage of these base classes as abstract interfaces for hardware device control.
Updated LaserBase, RemoteFocusBase, and ShutterBase to inherit from ABC and define key interface methods as abstractmethods. Removed unused initialize_laser method from LaserBase and SyntheticLaser.
Addresses #1139
StageBase now inherits from ABC and defines abstract methods for position reporting, absolute movement, axis movement, and stopping. Type hints and docstrings were added.
Implemented additional methods for synthetic device classes: camera (light sheet exposure calculation), galvo (turn_off), laser (set_power, turn_on, turn_off), stage (stop), and zoom (set_zoom). Added type annotations and abstract base class usage to ZoomBase and updated related zoom device classes.
Updated all device-related test files to use synthetic device classes instead of base classes for cameras, DAQs, filter wheels, galvos, lasers, remote focus, shutters, and zoom devices. Added a stub 'stop' method to MCLStage.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1140 +/- ##
===========================================
+ Coverage 53.53% 53.58% +0.05%
===========================================
Files 194 194
Lines 22374 22460 +86
===========================================
+ Hits 11978 12036 +58
- Misses 10396 10424 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@annie-xd-wang - I managed to spend a little time trying to wrap this up. Could you please double-check that I have 1) identified all of the proper interface methods and 2) that every established device has these methods implemented (either by calling the super() method, or just being a |
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the base classes for several device types to use Python's abc module, making them proper abstract base classes. This ensures consistent interfaces across hardware implementations and eliminates code duplication.
Key Changes:
- Converted
CameraBase,DAQBase,FilterWheelBase,GalvoBase,ShutterBase,StageBase,RemoteFocusBase,LaserBase, andZoomBaseto abstract base classes with@abstractmethoddecorators - Updated all test files to use synthetic implementations instead of base classes (which can no longer be instantiated)
- Moved common functionality like
load_imagesfrom synthetic camera to base camera class
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/navigate/model/devices/*/base.py |
Added ABC inheritance and abstract method decorators to enforce interface contracts |
src/navigate/model/devices/*/synthetic.py |
Implemented required abstract methods in synthetic device classes |
test/model/devices/*/test_*.py |
Updated tests to use concrete synthetic implementations instead of abstract base classes |
docs/source/index.rst |
Updated documentation with new badges and improved formatting |
docs/source/03_contributing/01_contributing_guidelines/01_contributing_guidelines.rst |
Enhanced contributing guidelines with better structure and hardware abstraction documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Test minimum boundary | ||
| move_dict[f"{axis}_abs"] = axis_min - 1.5 | ||
| abs_dict[axis] = axis_min - 1.5 | ||
| assert stage.verify_abs_position(move_dict) == abs_dict | ||
|
|
||
| # Test maximum boundary | ||
| move_dict[f"{axis}_abs"] = axis_max + 1.5 | ||
| abs_dict[axis] = axis_max + 1.5 | ||
| assert stage.verify_abs_position(move_dict) == abs_dict |
There was a problem hiding this comment.
[nitpick] The added comments improve readability, but the test logic appears to expect that positions outside the limits are accepted when stage_limits is False. Consider adding assertions that verify the stage_limits flag is indeed False before these boundary tests to make the test intent clearer.
Copilot uses AI. Check for mistakes.
| laser = SyntheticLaser(microscope_name, None, model.configuration, 0) | ||
|
|
||
| funcs = ["set_power", "turn_on", "turn_off", "close", "initialize_laser"] | ||
| funcs = ["set_power", "turn_on", "turn_off", "close"] |
There was a problem hiding this comment.
The removal of 'initialize_laser' from the function list suggests this method was removed from the base class. Ensure this change aligns with the abstract base class refactoring and that initialization logic is properly handled elsewhere.
Copilot uses AI. Check for mistakes.
| # TODO: self.num_of_frame is not defined in the base class | ||
| if idx >= self.num_of_frame: |
There was a problem hiding this comment.
This TODO comment indicates a potential issue where self.num_of_frame may not be properly defined in the base class. This should be resolved rather than left as a TODO, especially since the attribute is initialized in the __init__ method at line 166.
Copilot uses AI. Check for mistakes.
|
|
||
| def turn_off(self) -> None: | ||
| """Turn off the laser""" | ||
|
|
There was a problem hiding this comment.
[nitpick] There's an unnecessary blank line between the docstring and the pass statement. Remove the blank line for consistent formatting.
Copilot uses AI. Check for mistakes.
2 participants
This pull request refactors the base classes for several device types (camera, DAQ, filter wheel, and galvo) in the
navigatemodel to use Python'sabcmodule, making them proper abstract base classes. This enforces a consistent interface for all subclasses and clarifies which methods must be implemented by hardware-specific implementations. Additionally, some type annotations and docstrings have been improved, and redundant or duplicated code has been removed from subclasses.Abstract base class refactoring and interface enforcement:
Converted
CameraBase,DAQBase,FilterWheelBase, andGalvoBaseto inherit fromABCand use@abstractmethodfor key interface methods, ensuring that hardware-specific subclasses implement required methods. [1] [2] [3] [4] [5] [6] [7] [8]Added or updated abstract methods in base classes:
CameraBase:get_new_frame,initialize_image_series,close_image_series,set_line_interval,set_exposure_time,set_trigger_mode, andcalculate_light_sheet_exposure_timenow marked as abstract. [1] [2] [3]DAQBase:stop_acquisition,prepare_acquisition,run_acquisition, andwait_acquisition_donenow marked as abstract.FilterWheelBase:set_filternow marked as abstract.GalvoBase:turn_offnow marked as abstract.Code cleanup and improvements:
load_imagesimplementation fromSyntheticCamera, as this logic is now handled in the abstract base class.These changes help ensure that all device subclasses provide the necessary functionality, make the codebase more robust and maintainable, and reduce code duplication.