feat: Multi-camera support foundation - schema, hardware bindings, channel config UI #450
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces foundational schema models for multi-camera support, adding camera/filter wheel registries and channel groups for acquisition. The implementation is backward-compatible with v1.0 configs.
Changes:
- New models:
CameraRegistryConfig,FilterWheelRegistryConfig,ChannelGroupwith synchronization modes - Added
channel_groupsfield toGeneralChannelConfig(defaults to empty list) - Migration utilities for v1.0 to v1.1 (minimal - just adds channel_groups)
- Repository methods for new registry configs
- Comprehensive test coverage for new models
- Documentation for multi-camera support plan and existing v1.0 config system
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
control/models/camera_registry.py |
New model for camera name→serial mapping |
control/models/filter_wheel_config.py |
New model for filter wheel definitions |
control/models/acquisition_config.py |
Added ChannelGroup models and channel_groups field |
control/models/__init__.py |
Exports for new v1.1 models |
control/core/config/repository.py |
Load/save methods for registries |
control/core/config/migration.py |
Minimal v1.0→v1.1 migration utilities |
tests/control/test_v1_1_models.py |
Comprehensive tests for new models |
machine_configs/*.yaml.example |
Example configs for registries |
docs/pending/multi-camera-support.md |
Design document for multi-camera implementation |
docs/configuration-*.md |
Documentation for v1.0 config system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Filter Wheel Registry Configuration (v1.1) | ||
| # | ||
| # This file defines all filter wheels in the system with their positions | ||
| # and installed filters. Acquisition channels reference filter wheels by name. |
There was a problem hiding this comment.
The comment says "Acquisition channels reference filter wheels by name" but this is misleading because the current v1.0 AcquisitionChannel model doesn't have filter_wheel/filter_position fields. The current implementation uses emission_filter_wheel_position which is a Dict[int, int] mapping wheel ID to position, not a named reference. This comment should be updated to reflect the current implementation or marked as a future feature.
| # and installed filters. Acquisition channels reference filter wheels by name. | |
| # and installed filters. Acquisition channels reference filter wheels by | |
| # numeric ID (see the `id` field) via `emission_filter_wheel_position`, | |
| # not by name. |
| Key changes from v1.0: | ||
| - `display_color` moved from `camera_settings` to channel level | ||
| - `camera` field references camera by name (optional for single-camera systems) | ||
| - `camera_settings` is now a single object (not `Dict[str, CameraSettings]`) | ||
| - `filter_wheel` and `filter_position` replace `emission_filter_wheel_position` | ||
| - New `channel_groups` section |
There was a problem hiding this comment.
The documented v1.1 schema changes do not match the actual model implementation in acquisition_config.py. The documentation describes:
display_colormoved to channel levelcamerafield added to channelcamera_settingsas single object (not Dict)filter_wheelandfilter_positionfields
However, the AcquisitionChannel model in acquisition_config.py still uses the v1.0 schema with:
camera_settings: Dict[str, CameraSettings](still a Dict)emission_filter_wheel_position: Optional[Dict[int, int]](not filter_wheel/filter_position)- No
camerafield - No
display_colorfield on channel (it's in CameraSettings)
Either the models need to be updated to match this documentation, or this documentation needs to be clarified as a future design plan rather than current implementation.
| ```yaml | ||
| version: 1.1 | ||
| channels: | ||
| - name: BF LED matrix full | ||
| display_color: '#FFFFFF' # MOVED: was in camera_settings | ||
| # camera: null # OPTIONAL: omit for single-camera systems | ||
| camera_settings: # CHANGED: single object, not Dict | ||
| exposure_time_ms: 20.0 | ||
| gain_mode: 10.0 | ||
| pixel_format: null | ||
| filter_wheel: null # NEW: references filter_wheels.yaml by name | ||
| filter_position: null # NEW: position in that wheel | ||
| illumination_settings: | ||
| illumination_channels: | ||
| - BF LED matrix full | ||
| intensity: | ||
| BF LED matrix full: 20.0 | ||
| z_offset_um: 0.0 | ||
| confocal_settings: null | ||
| confocal_override: null | ||
| channel_groups: [] # Empty for single-camera, no groups needed | ||
| ``` |
There was a problem hiding this comment.
This example YAML shows the v1.1 schema with camera, display_color at channel level, and camera_settings as a single object. However, the actual AcquisitionChannel model in acquisition_config.py uses v1.0 schema with Dict[str, CameraSettings]. The code will fail to parse this YAML structure because it doesn't match the Pydantic model definition.
| ```yaml | ||
| version: 1.1 | ||
| channels: | ||
| - name: BF LED matrix full | ||
| display_color: '#FFFFFF' | ||
| camera: "Main Camera" # REQUIRED for multi-camera: references cameras.yaml | ||
| camera_settings: | ||
| exposure_time_ms: 20.0 | ||
| gain_mode: 10.0 | ||
| pixel_format: null | ||
| filter_wheel: null | ||
| filter_position: null | ||
| illumination_settings: | ||
| illumination_channels: | ||
| - BF LED matrix full | ||
| intensity: | ||
| BF LED matrix full: 20.0 | ||
| z_offset_um: 0.0 | ||
| confocal_settings: null | ||
| confocal_override: null | ||
| - name: Fluorescence 488 nm Ex | ||
| display_color: '#1FFF00' | ||
| camera: "Side Camera" # Different camera for simultaneous capture | ||
| camera_settings: | ||
| exposure_time_ms: 20.0 | ||
| gain_mode: 10.0 | ||
| pixel_format: null | ||
| filter_wheel: "Emission Filter Wheel" | ||
| filter_position: 2 | ||
| illumination_settings: | ||
| illumination_channels: | ||
| - Fluorescence 488 nm Ex | ||
| intensity: | ||
| Fluorescence 488 nm Ex: 20.0 | ||
| z_offset_um: 0.0 | ||
| confocal_settings: null | ||
| confocal_override: null | ||
| channel_groups: | ||
| - name: "Dual BF + GFP" | ||
| synchronization: simultaneous | ||
| channels: | ||
| - name: "BF LED matrix full" # Main Camera | ||
| offset_us: 0 # Reference channel | ||
| - name: "Fluorescence 488 nm Ex" # Side Camera | ||
| offset_us: 100 # 100μs delay | ||
| - name: "Standard Fluorescence" | ||
| synchronization: sequential | ||
| channels: | ||
| - name: "Fluorescence 488 nm Ex" | ||
| - name: "Fluorescence 561 nm Ex" | ||
| - name: "Fluorescence 638 nm Ex" | ||
| ``` |
There was a problem hiding this comment.
This multi-camera example shows v1.1 schema structure that doesn't match the actual model implementation. The camera, filter_wheel, and filter_position fields, along with flattened camera_settings, are not present in the current AcquisitionChannel model.
| class AcquisitionChannel(BaseModel): | ||
| """A single acquisition channel configuration.""" | ||
| name: str = Field(..., description="Channel display name") | ||
| display_color: str = Field('#FFFFFF', description="Hex color for UI visualization") | ||
| # Camera assignment (optional for single-camera systems) | ||
| camera: Optional[str] = Field( | ||
| None, | ||
| description="Camera name (references cameras.yaml). Optional for single-camera systems." | ||
| ) | ||
| camera_settings: CameraSettings = Field(..., description="Camera settings") | ||
| # Filter wheel assignment (optional) | ||
| filter_wheel: Optional[str] = Field( | ||
| None, description="Filter wheel name (references filter_wheels.yaml)" | ||
| ) | ||
| filter_position: Optional[int] = Field( | ||
| None, description="Position in filter wheel" | ||
| ) | ||
| # Illumination | ||
| illumination_settings: IlluminationSettings | ||
| # Confocal (optional) | ||
| confocal_settings: Optional[ConfocalSettings] = None | ||
| confocal_override: Optional[AcquisitionChannelOverride] = None | ||
| model_config = {"extra": "forbid"} | ||
| # Convenience properties | ||
| @property | ||
| def exposure_time(self) -> float: | ||
| return self.camera_settings.exposure_time_ms | ||
| @exposure_time.setter | ||
| def exposure_time(self, value: float) -> None: | ||
| self.camera_settings.exposure_time_ms = value | ||
| @property | ||
| def analog_gain(self) -> float: | ||
| return self.camera_settings.gain_mode | ||
| @analog_gain.setter | ||
| def analog_gain(self, value: float) -> None: | ||
| self.camera_settings.gain_mode = value | ||
There was a problem hiding this comment.
The documented model structure for v1.1 AcquisitionChannel does not match the actual implementation. The actual model in acquisition_config.py still uses:
camera_settings: Dict[str, CameraSettings](not single CameraSettings)emission_filter_wheel_position: Optional[Dict[int, int]](not filter_wheel/filter_position)- No
camerafield - No
display_colorat channel level
This documentation appears to describe a planned schema but not the current implementation.
| logger.debug(f"Could not generate default configs (module not available): {e}") | ||
| except FileNotFoundError as e: | ||
| # Expected if illumination config doesn't exist yet | ||
| logger.warning(f"Could not generate default configs (missing required config): {e}") |
There was a problem hiding this comment.
The improved exception handling is good, but the change from a generic Exception to specific exceptions (ImportError, FileNotFoundError) could miss other legitimate exceptions that might occur during default config generation. Consider whether other exceptions (like ValidationError from pydantic) should also be caught here, or if uncaught exceptions should be allowed to bubble up for debugging purposes.
| logger.warning(f"Could not generate default configs (missing required config): {e}") | |
| logger.warning(f"Could not generate default configs (missing required config): {e}") | |
| except ValidationError as e: | |
| # Expected if generated default configs fail Pydantic validation | |
| logger.warning(f"Could not generate default configs (validation error): {e}") |
hongquanli
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 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.
hongquanli
force-pushed
the
feat/multi-camera-support
branch
from
73f24f9 to
f6137ae
Compare
Add documentation for the YAML-based configuration system introduced in PRs #417 and #441: - configuration-system.md: Main guide covering architecture, machine configs (illumination, confocal, camera mappings), user profiles (general.yaml, objective.yaml), merge logic, and best practices - configuration-api.md: Developer reference for ConfigRepository API, Pydantic models, utility functions, and access patterns - configuration-migration.md: Migration guide from legacy XML/JSON format including auto-migration, manual migration script usage, and troubleshooting - Update automation.md to reference new configuration docs Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix has_legacy_configs_to_migrate() signature (takes profile and base_path) - Clarify confocal_config.yaml structure matches Pydantic model - Add note about .example file format difference Co-Authored-By: Claude Opus 4.5 <[email protected]>
The method signature is get_channels(objective), not get_channels(). Updated the widget example to show correct usage. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Mark camera_mappings.yaml as optional (for multi-camera setups) - Mark intensity_calibrations/ as optional in directory diagram - Mark illumination_channel_config.yaml as required Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit introduces the foundational schema changes for multi-camera acquisition support: New models: - CameraRegistryConfig: Maps camera names to serial numbers - FilterWheelRegistryConfig: Defines filter wheels and positions - ChannelGroup: Groups channels for simultaneous/sequential acquisition - ChannelGroupEntry: Per-channel settings within a group (offset_us) - SynchronizationMode: Enum for simultaneous vs sequential capture Schema updates (v1.1): - GeneralChannelConfig now supports channel_groups - Version field changed from int to Union[int, float] for 1.1 support - Added validate_channel_group() for channel group validation ConfigRepository additions: - get_camera_registry() / save_camera_registry() - get_filter_wheel_registry() / save_filter_wheel_registry() - get_camera_names() / get_filter_wheel_names() Example configs: - cameras.yaml.example - filter_wheels.yaml.example Documentation: - docs/pending/multi-camera-support.md with full implementation plan Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add zero-config goal for single-camera systems - Add behavior table showing camera field requirements - Document camera field as Optional[str] - Add separate single-camera and multi-camera YAML examples - Mark default camera behavior as resolved design decision Co-Authored-By: Claude Opus 4.5 <[email protected]>
Addresses code review feedback: Validation improvements: - Add uniqueness validators for camera names/SNs in CameraRegistryConfig - Add uniqueness validators for filter wheel names/IDs - Add min_length=1 to name and serial_number fields - Add ge=0 for hardware IDs, ge=1 for filter positions - Add gt=0 for exposure_time_ms, ge=0 for gain_mode and offset_us - Add intensity range validation (0-100) - Add min_length=1 for ChannelGroup.channels Error handling improvements: - Narrow exception handler in repository.py (ImportError, FileNotFoundError only) - Add defensive handling in get_config_version for None/invalid input - Use copy.deepcopy in migration functions to avoid nested mutations Tests: - Add comprehensive test suite for v1.1 models (71 tests) - Tests for CameraRegistryConfig, FilterWheelRegistryConfig - Tests for ChannelGroup, ChannelGroupEntry, SynchronizationMode - Tests for migration utilities - Tests for validation constraints Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add a new dialog (Settings > Channel Configuration) for editing acquisition channel settings in user profiles (general.yaml). Features: - Table-based editor with columns: Enabled, Name, Illumination, Camera, Filter Wheel, Filter Position, Color - Smart column visibility: Camera column hidden when ≤1 camera, Filter Wheel column hidden when ≤1 wheel - Add/Remove/Move Up/Move Down channel management - Color picker for display colors - Add `enabled` field to AcquisitionChannel model The dialog edits user-facing acquisition settings, separate from the hardware-level IlluminationChannelConfiguratorDialog. Co-Authored-By: Claude Opus 4.5 <[email protected]>
UI Polish: - Fix "Enabled" column header (was blank) - Fix color column width (60px fixed, QPushButton style) - Name column now stretches instead of Color column - Reduce dialog minimum size to 700x400 Filter Wheel/Position Logic: - Hide Filter Wheel column when single wheel (auto-assign to all channels) - Filter Position defaults to Position 1 (no N/A when wheel available) - Check USE_EMISSION_FILTER_WHEEL for implicit "Emission" wheel Bug Fixes: - Disabled channels now hidden from live controller dropdown - Add Channel dialog shows filter position options (fixed PyQt5 boolean check) New Features: - Export/Import buttons for channel configurations Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add UI for configuring filter wheel position names via Settings > Advanced > Filter Wheel Configuration. Users can now assign meaningful names to filter positions (e.g., "DAPI emission" instead of "Position 1"). - Add FilterWheelConfiguratorDialog to widgets.py - Add menu item in gui_hcs.py Advanced menu - Creates default "Emission" wheel with 8 positions if none exists - Saves configuration to machine_configs/filter_wheels.yaml Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Mark Phase 1 (Schema and Models) as complete - Update Phase 2 (UI Integration) with completed items: - Acquisition Channel Configuration dialog - Filter Wheel Configuration dialog - Filter wheel/position selector - Disabled channels filtering - List remaining Phase 2 work items Co-Authored-By: Claude Opus 4.5 <[email protected]>
Critical fixes: - Fix _populate_table() -> _load_channels() in import config - Add error handling to _save_changes() with user feedback High priority fixes: - Narrow exception types in export config (PermissionError, OSError) - Add logging to import/export error handling - Add logging to FilterWheelConfiguratorDialog save errors Medium priority fixes: - Show user feedback when config missing in _load_channels() - Add wheel_id validation with try-except in migration.py (3 places) - Add error logging to _save_yaml in repository.py - Add warning for None filter position when wheel selected Code quality: - Remove redundant import control._def (already imported at module level) - Fix type hint: current_position: int -> Optional[int] Test coverage: - Add migration edge case tests (display_color extraction, camera_settings flattening, filter wheel conversion, confocal_override migration) - Add filter_wheel_registry migration tests - Add enabled channel filtering tests Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Extract _map_wheel_id_to_name() helper for wheel ID validation - Extract _migrate_confocal_override_v1_to_v1_1() helper function - Consolidate duplicate filter position population logic in widgets - Remove redundant code paths in AcquisitionChannelConfiguratorDialog - Net reduction of 40 lines while maintaining all functionality Co-Authored-By: Claude Opus 4.5 <[email protected]>
Since v1.0 was never released, remove the v1.0 -> v1.1 migration path. Now only pre-v1 (legacy XML/JSON) -> v1.1 migration is supported via tools/migrate_acquisition_configs.py. Changes: - Delete control/core/config/migration.py (v1.0 -> v1.1 migration code) - Update migrate_acquisition_configs.py to produce v1.1 format directly - Remove TestMigrationUtilities from test_v1_1_models.py - Update all tests to use v1.1 schema: - display_color at AcquisitionChannel level (not in CameraSettings) - camera_settings as single object (not Dict) - filter_wheel/filter_position instead of emission_filter_wheel_position Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add camera registry (cameras.yaml) documentation - Add filter wheel registry (filter_wheels.yaml) documentation - Update ConfigRepository API docs with new methods - Mark camera_mappings.yaml as legacy - Update version references from 1 to 1.1 in examples Co-Authored-By: Claude Opus 4.5 <[email protected]>
hongquanli
force-pushed
the
feat/multi-camera-support
branch
from
e1fdb7d to
c2284f3
Compare
Change exclude_none=True when saving configs, so optional fields like camera, filter_wheel, confocal_settings are omitted when null. This produces cleaner YAML files, especially for single-camera setups where "camera: null" was confusing. Co-Authored-By: Claude Opus 4.5 <[email protected]>
For single filter wheel setups, store filter_wheel="auto" in YAML instead of the wheel name. This distinguishes from None (no wheel) while keeping the actual wheel name hidden from users in config files. UI behavior: - User sees actual wheel name in dropdown (never sees "auto") - "auto" is stored in YAML, resolved to actual wheel at runtime Changes: - widgets.py: Store "auto" when single wheel selected, display actual wheel name in UI by resolving "auto" -> _auto_wheel - migrate_acquisition_configs.py: Use filter_wheel="auto" for migrated configs that have filter wheels Co-Authored-By: Claude Opus 4.5 <[email protected]>
…unter Code quality fixes: - validate_channel_group: Filter out None cameras before duplicate checking, warn when simultaneous mode channels have no camera assigned - acquisition_config.py: Fix module docstring to correctly describe confocal fields as nested inside confocal_settings Error handling improvements: - migrate_acquisition_configs.py: Add try/except to save_yaml for file write errors, add warning when XML parse returns empty but file exists - repository.py: Add catches for PermissionError, OSError, and general Exception in load_profile default config generation UI improvements: - widgets.py: Add module-level logger, warn when filter wheel not found in registry (fallback to default positions), add bounds check before channel deletion, show dialog when filter wheel has no position selected, use config_repo.save_filter_wheel_registry instead of direct yaml.dump Documentation updates: - configuration-system.md: Update YAML examples to v1.1 format with single camera_settings object, filter_wheel/filter_position fields, and display_color at channel level Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add comprehensive tests for v1.1 registry methods: - get_camera_registry / save_camera_registry - get_filter_wheel_registry / save_filter_wheel_registry - get_camera_names / get_filter_wheel_names - Caching behavior and None returns for missing files Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add comprehensive tests for: - Field validation constraints (hex color, iris range, intensity, exposure, gain) - GeneralChannelConfig.get_group_by_name (found/not found) - GeneralChannelConfig.get_group_names - IlluminationChannel validation (name, port, wavelength) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Update configuration-api.md examples from v1.0 to v1.1 schema - Single camera_settings object (not dict by camera ID) - Single illumination_channel with scalar intensity - Correct confocal_settings field names - Fix confocal_override example in configuration-system.md - Use confocal_filter_wheel and confocal_filter_position - Remove obsolete nested camera_settings by ID - Add field validation constraints documentation - Code simplifications from review: - Consolidate f-string in acquisition_config.py - Simplify validation logic in camera_registry.py - Remove redundant variables in filter_wheel_config.py - Simplify save_yaml in migrate_acquisition_configs.py Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add FilterWheelType enum with EXCITATION and EMISSION values - Add optional type field to FilterWheelDefinition model - Add helper methods: get_wheels_by_type, get_excitation_wheels, get_emission_wheels - Export FilterWheelType from models package - Update documentation with type field in filter_wheels.yaml example - Add tests for type field and type-based getters Co-Authored-By: Claude Opus 4.5 <[email protected]>
Excitation and emission filter wheels serve different purposes: - Emission filter wheels (0-1 per system): Referenced by acquisition channels via filter_wheel/filter_position in user profile configs - Excitation filter wheels (rare): Referenced by illumination channels via excitation_filter_wheel/excitation_filter_position in machine config Changes: - Add excitation_filter_wheel and excitation_filter_position fields to IlluminationChannel model - Update documentation to explain the distinction - Add tests for excitation filter wheel fields Co-Authored-By: Claude Opus 4.5 <[email protected]>
…port Add hardware bindings system with source-qualified references (e.g., "confocal.1", "standalone.Emission") to support multi-camera setups where each camera may need a different emission filter wheel. Key changes: - Add HardwareBindingsConfig model with load-time reference validation - Add FilterWheelReference for parsing "source.identifier" format - Add filter wheel aggregation methods to ConfigRepository - Add get_effective_emission_wheel() for resolving camera→wheel bindings - Refactor validators to use mode='before' (no input mutation) - Extract shared validation into helper functions (DRY) - Add helper methods to ConfocalConfig for API consistency - Treat missing cameras.yaml as single-camera system for implicit binding Co-Authored-By: Claude Opus 4.5 <[email protected]>
Type Design Improvements: - Add FilterWheelSource enum for type-safe source specification - Store Dict[int, FilterWheelReference] instead of Dict[int, str] - Make FilterWheelReference frozen (immutable) with __hash__ - Add field serializer for human-readable YAML output Error Handling Fixes: - Remove silent failures in get_emission_wheel_ref/get_all_emission_wheel_refs (now just dict lookups since validation happens at load time) - Upgrade resolve_wheel_reference() log level from DEBUG to WARNING - Add type validation in apply_single_filter_wheel_defaults() New Tests: - test_parse_name_with_dots: names containing dots - test_parse_empty_source_rejected: malformed ".1" references - test_reference_is_frozen: immutability verification - test_reference_hashable: dict key usage - test_source_enum_values: enum validation - test_serialization_to_strings: YAML output format - test_remove_binding: binding removal - test_confocal_config_single_wheel_defaults: parity with standalone Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Document new hardware_bindings.yaml for camera→wheel mappings - Explain source-qualified reference format (confocal.1, standalone.name) - Add notes to filter_wheels.yaml and confocal_config.yaml about sources - Update architecture overview file tree Co-Authored-By: Claude Opus 4.5 <[email protected]>
Since v1.0 was never released, rename current schema to v1.0. Remove camera_mappings.yaml from docs (deprecated/unreleased). Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove objective-specific fields (intensity, exposure_time_ms, gain_mode) from general.yaml example. Move z_offset_um to channel level. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…wheel Key changes: - Camera field changed from string name to int ID (null for single-camera) - Removed confocal_filter_wheel/confocal_filter_position from ConfocalSettings - Filter wheel resolved via hardware_bindings.yaml based on camera ID - Moved z_offset_um from IlluminationSettings to AcquisitionChannel level - Removed confocal_settings from AcquisitionChannel (iris in confocal_override) - Updated all tests to reflect new model structure This simplifies the config by having a single filter_position field that works with hardware_bindings.yaml to resolve the actual wheel. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- cameras.yaml now shows id field (used in channel configs) - general.yaml uses camera ID (int) not name (string) - filter_position resolved via hardware_bindings.yaml - confocal_override only has iris settings (no filter wheel fields) - Updated all examples and field tables Co-Authored-By: Claude Opus 4.5 <[email protected]>
…hema - Remove confocal_filter_wheel/confocal_filter_position from ConfocalSettings (filter wheel resolved via hardware_bindings.yaml) - Move z_offset_um from IlluminationSettings to AcquisitionChannel level - Change camera parameter from string name to int ID - Update version numbers from 1.1 to 1.0 - Update test fixtures to use new schema format Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
As we already have entries of camera model and serial number in the ini file, I think we should either 1) make this file compulsory and remove all the relevant information from ini file, or 2) remove this file and only keep the camera hardware information in the ini file (though we may move to yaml in the future), and only use new yaml file for defining camera bindings.
This way we will only have one source of truth. I would prefer option 2 so we won't make things over complicated
There was a problem hiding this comment.
Was thinking we can leave any modifications related to 2 cameras, including how the hardware yaml files are generated to the next PR.
- Fix merged docstring lines in __init__.py - Fix merged __all__ comments - Merge concatenated f-strings in filter_wheel_config.py Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Change camera from string to int ID in test fixtures - Move z_offset_um from IlluminationSettings to channel level - Update version assertions from 1.1 to 1.0 Co-Authored-By: Claude Opus 4.5 <[email protected]>
hongquanli
changed the title
hongquanli
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,11 @@ | |||
| version: 1.1 | |||
| filter_wheels: | |||
| - positions: | |||
There was a problem hiding this comment.
The filter wheel definition is missing required 'type' field. According to the schema, FilterWheelDefinition requires a 'type' field (excitation or emission). The example file shows 'type: emission' but this file is missing it.
| - positions: | |
| - type: emission | |
| positions: |
There was a problem hiding this comment.
[Claude Code] Skipped - file does not exist in current branch state. This may be from a stale review.
There was a problem hiding this comment.
[Claude Code] Fixed in commit f3b6d32 - added name, id, and type fields to filter_wheels.yaml, set version to 1.0.
| Unit tests for v1.0 configuration models. | ||
|
|
||
| Tests the new models introduced in schema v1.0: |
There was a problem hiding this comment.
Corrected spelling in docstring: 'v1.0' should be 'v1.1' to match the filename test_v1_1_models.py.
| Unit tests for v1.0 configuration models. | |
| Tests the new models introduced in schema v1.0: | |
| Unit tests for v1.1 configuration models. | |
| Tests the new models introduced in schema v1.1: |
There was a problem hiding this comment.
[Claude Code] Skipped - file does not exist in current branch state. This may be from a stale review.
There was a problem hiding this comment.
[Claude Code] Already fixed - file says 'v1.0' in docstring. The filename test_v1_1_models.py is a misnomer from original planning but the content is correct.
| except: | ||
| except (AttributeError, RuntimeError): | ||
| # main_window may be None or already closed | ||
| pass |
There was a problem hiding this comment.
Bare 'except' block was replaced with specific exception types, which is good. However, consider logging these exceptions since silently catching AttributeError and RuntimeError could hide legitimate issues beyond 'window is None or closed'.
| pass | |
| logger.warning( | |
| "Failed to close main_window in ConfigEditorBackwardsCompatible.apply_and_exit; " | |
| "this may be expected if the window is None or already closed.", | |
| exc_info=True, | |
| ) |
There was a problem hiding this comment.
[Claude Code] Skipped - low value. The bare except already handles expected close failures when window is None or already closed. Adding logging for this expected case would add noise.
There was a problem hiding this comment.
[Claude Code] Skipped - low value change. The exception is expected when window is already closed; logging at WARNING level would generate noise for normal shutdown scenarios.
| def _populate_filter_positions_for_combo( | ||
| combo: QComboBox, | ||
| channel_wheel: Optional[str], | ||
| config_repo, | ||
| current_position: Optional[int] = None, | ||
| ) -> None: |
There was a problem hiding this comment.
Missing type hint for 'config_repo' parameter. Should specify ConfigRepository type for better type safety and IDE support.
There was a problem hiding this comment.
[Claude Code] Skipped - low value style change. Type hints are helpful but not critical for this internal method.
There was a problem hiding this comment.
[Claude Code] Skipped - low value style change. Type hints are helpful but not critical for this internal method.
| ``` | ||
| """ | ||
|
|
||
| version: float = Field(1.0, description="Configuration format version") |
There was a problem hiding this comment.
Version field default is 1.0 but documentation and examples show 1.1. This inconsistency could cause confusion. Either update the default to 1.1 or clarify why 1.0 is used.
| version: float = Field(1.0, description="Configuration format version") | |
| version: float = Field(1.1, description="Configuration format version") |
There was a problem hiding this comment.
[Claude Code] Skipped - file does not exist in current branch state. This may be from a stale review.
There was a problem hiding this comment.
[Claude Code] Intentional - version 1.0 is correct. We renamed the schema back to v1.0 since the original v1.0 was never released. The PR description has been updated to reflect this.
| import logging | ||
| from typing import Dict, List, Optional, Set, TYPE_CHECKING | ||
| from enum import Enum | ||
| from typing import Dict, List, Optional, Set, Union, TYPE_CHECKING |
There was a problem hiding this comment.
Import of 'Dict' is not used.
| from typing import Dict, List, Optional, Set, Union, TYPE_CHECKING | |
| from typing import List, Optional, Set, Union, TYPE_CHECKING |
There was a problem hiding this comment.
[Claude Code] False positive - Dict IS used in this file at lines 59, 75, 88, 89, 91 (e.g., Dict[str, CameraSettings], Dict[int, int]).
There was a problem hiding this comment.
[Claude Code] Fixed in commit f3b6d32 - removed unused Dict import.
| from typing import Dict, List, Optional, Set, Union, TYPE_CHECKING | ||
|
|
||
| from pydantic import BaseModel, Field | ||
| from pydantic import BaseModel, Field, field_validator |
There was a problem hiding this comment.
Import of 'field_validator' is not used.
| from pydantic import BaseModel, Field, field_validator | |
| from pydantic import BaseModel, Field |
There was a problem hiding this comment.
[Claude Code] False positive - field_validator is not imported in this file. Line 16 only imports BaseModel, Field from pydantic.
There was a problem hiding this comment.
[Claude Code] Fixed in commit f3b6d32 - removed unused field_validator import.
| from control.models.hardware_bindings import ( | ||
| FilterWheelSource, | ||
| FilterWheelReference, | ||
| HardwareBindingsConfig, | ||
| FILTER_WHEEL_SOURCE_CONFOCAL, | ||
| FILTER_WHEEL_SOURCE_STANDALONE, | ||
| ) |
There was a problem hiding this comment.
Import of 'FilterWheelSource' is not used.
There was a problem hiding this comment.
[Claude Code] False positive - FilterWheelSource is not imported in repository.py. The imports at lines 25-37 do not include FilterWheelSource.
There was a problem hiding this comment.
[Claude Code] Fixed in commit f3b6d32 - removed unused FilterWheelSource import.
| ) | ||
| from control.models.camera_registry import CameraRegistryConfig, CameraDefinition | ||
| from control.models.filter_wheel_config import FilterWheelRegistryConfig, FilterWheelDefinition, FilterWheelType | ||
| from control.models.confocal_config import ConfocalConfig |
There was a problem hiding this comment.
Import of 'ConfocalConfig' is not used.
| from control.models.confocal_config import ConfocalConfig |
There was a problem hiding this comment.
[Claude Code] False positive - ConfocalConfig is not imported in test_repository.py. The imports at lines 10-17 do not include ConfocalConfig.
There was a problem hiding this comment.
[Claude Code] Fixed in commit f3b6d32 - removed unused ConfocalConfig import.
| FILTER_WHEEL_SOURCE_STANDALONE = FilterWheelSource.STANDALONE.value | ||
|
|
||
|
|
||
| class FilterWheelReference(BaseModel): |
There was a problem hiding this comment.
This class implements hash, but does not implement eq.
There was a problem hiding this comment.
[Claude Code] Skipped - file does not exist in current branch state. This may be from a stale review.
There was a problem hiding this comment.
[Claude Code] False positive - FilterWheelReference uses model_config = {"frozen": True}, which causes Pydantic to auto-generate eq. The explicit hash is required because frozen models need hashability for use as dict keys.
Updates documentation files to accurately reflect the implemented v1.0 schema instead of the originally planned v1.1: configuration-api.md: - Change all "v1.1" references to "v1.0" - Fix ConfocalSettings example (removed confocal_filter_wheel/position) - Fix z_offset_um location (channel level, not IlluminationSettings) - Update validation constraints (remove non-existent fields) - Add id field to CameraDefinition example - Update version numbers in code examples configuration-system.md: - Add camera_settings (required) to general.yaml example - Add intensity (required) to illumination_settings example - Update fields table with camera_settings and intensity pending/multi-camera-support.md: - Update all v1.1 references to v1.0 - Fix ConfocalSettings class (iris only, no filter wheel fields) - Update camera field from Optional[str] to Optional[int] - Update migration section (legacy to v1.0, not v1.0 to v1.1) - Fix confocal integration design decision Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix filter_wheels.yaml: add required type/name/id fields, version 1.0 - Fix filter_wheels.yaml.example: update to version 1.0, add type field - Remove unused Dict import from acquisition_config.py - Remove unused field_validator import from acquisition_config.py - Remove unused FilterWheelSource import from repository.py - Remove unused ConfocalConfig import from test_repository.py Co-Authored-By: Claude Opus 4.5 <[email protected]>
Resolved conflicts in migrate_acquisition_configs.py: - Used master's mode='json' in model_dump for proper enum serialization - Merged master's two-pass approach with deduplication (all_channels_by_name) - Kept v1.0 schema format for general.yaml generation - Used master's laser_af_configs path
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 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.
Updates submodule to include memory leak fix for live refresh. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Schema is v1.0, not v1.1. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary
This PR introduces foundational changes for multi-camera acquisition support, including schema models, hardware bindings, UI dialogs, and configuration management.
New Configuration Files
cameras.yaml(optional): Maps user-friendly camera names to hardware serial numbers. Only required for multi-camera systems.filter_wheels.yaml: Defines filter wheels with positions and filter names. Auto-created with default "Emission" wheel whenUSE_EMISSION_FILTER_WHEEL=True.hardware_bindings.yaml(future): Will define camera-to-filter-wheel bindings for multi-camera setups.New Models
source.idorsource.nameformatSchema Updates (v1.0)
AcquisitionChannelmodel updated with new fields:camera: Optional integer ID referencingcameras.yaml(null for single-camera systems)filter_wheel: Optional string ("auto" uses hardware binding, or specify wheel name)filter_position: Optional integer position in the filter wheeldisplay_color: Moved fromcamera_settingsto channel levelcamera_settingssimplified to singleCameraSettingsobject (was Dict)ConfigRepository Additions
get_camera_registry()/save_camera_registry()get_filter_wheel_registry()/save_filter_wheel_registry()get_hardware_bindings()/save_hardware_bindings()get_camera_names()/get_filter_wheel_names()New UI Dialogs (Settings > Advanced)
Bug Fixes
USE_EMISSION_FILTER_WHEEL=TrueDocumentation
docs/pending/multi-camera-support.mddocs/configuration-system.mdanddocs/configuration-api.mdcameras.yaml.example,filter_wheels.yaml.exampleTest Coverage
test_multi_camera_models.pytest_repository.pywith camera/filter wheel registry testsBackward Compatibility
This is an additive change - existing configs continue to work:
camerais optional (null = use single available camera)filter_wheelis optional (auto-resolved via hardware bindings)Test Plan
🤖 Generated with Claude Code