feat: Refactor channel configuration to eliminate duplication across … #392
Conversation
…objectives - Add global channel definitions (channel_definitions.json) with two-stage mapping: - Numeric channel -> illumination source + excitation wavelength - Channel name -> numeric channel (fluorescence) or direct illumination source (for LED matrix) - Add per-objective settings for exposure, gain, intensity (stored in channel_settings.json) - Add Channel Configuration Editor dialog (Settings menu) - Add Advanced Channel Hardware Mapping dialog (Settings > Advanced menu) - Add enable/disable functionality for channels (disabled channels hidden from dropdowns) - Maintain backward compatibility with legacy XML format for acquisitions - Add input validation for new channel names (non-empty, unique) New Pydantic models: - ChannelType (fluorescence, led_matrix) - NumericChannelMapping - ChannelDefinition (with validation) - ObjectiveChannelSettings - ChannelDefinitionsConfig Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Rename channel_definitions.json to channel_definitions.default.json (tracked) - Add channel_definitions.json to .gitignore (user edits) - App copies default to user file on first run, then uses user file Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Also run black formatter on Python files. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the channel configuration system to eliminate duplication across objectives by introducing a two-tier architecture: global channel definitions stored in channel_definitions.json and per-objective settings in channel_settings.json.
Key changes include:
- Introduction of new Pydantic models (ChannelType, ChannelDefinition, ObjectiveChannelSettings, ChannelDefinitionsConfig) for type-safe configuration management
- Two new GUI dialogs for editing channel configurations and hardware mappings
- Enable/disable functionality for channels with automatic filtering in dropdowns
- Backward compatibility maintained through automatic XML generation
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| software/tests/control/gui_test_stubs.py | Updates test helper functions to use refactored configuration manager (contains typos) |
| software/docs/channel_configuration.md | Comprehensive documentation for the new channel configuration system |
| software/control/widgets.py | Adds ChannelEditorDialog, AddChannelDialog, and AdvancedChannelMappingDialog for GUI configuration editing; updates dropdown population to use enabled channels only |
| software/control/utils_config.py | Defines new Pydantic models for channel configuration with validation logic |
| software/control/microscope.py | Updates Microscope class to initialize ChannelConfigurationManager with configurations path (contains typos) |
| software/control/gui_hcs.py | Integrates new channel configuration dialogs into Settings menu; adds refresh logic for channel lists (contains typos) |
| software/control/core/multi_point_controller.py | Updates constructor to accept renamed parameter (contains typos) |
| software/control/core/channel_configuration_mananger.py | Major refactor to support dual-format configuration (JSON + XML), adds migration logic, implements enable/disable functionality |
| software/configurations/channel_definitions.default.json | Default channel definitions with hardware mappings (contains data inconsistencies) |
| software/.gitignore | Adds acquisition_configurations/ and configurations/channel_definitions.json to gitignore |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.piezo: Optional[PiezoStage] = microscope.addons.piezo_stage | ||
|
|
||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_manager | ||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger |
There was a problem hiding this comment.
The attribute reference contains a typo: "mananger" should be "manager". This misspelling is inconsistent with the correct spelling used elsewhere in the codebase.
| self.configuration_manager: ConfigurationManager = ConfigurationManager( | ||
| self.channel_configuration_manager, self.laser_af_settings_manager | ||
| self.configuration_mananger: ConfigurationManager = ConfigurationManager( | ||
| self.channel_configuration_mananger, self.laser_af_settings_manager |
There was a problem hiding this comment.
The parameter name contains a typo: "mananger" should be "manager". This misspelling is inconsistent with the correct spelling used elsewhere in the codebase.
| "name": "test test", | ||
| "type": "fluorescence", | ||
| "emission_filter_position": 3, | ||
| "display_color": "#FFFFFF", | ||
| "enabled": true, | ||
| "numeric_channel": 3, | ||
| "illumination_source": null, | ||
| "ex_wavelength": null | ||
| }, | ||
| { |
There was a problem hiding this comment.
This test channel ("test test") should not be present in a default configuration file that is tracked in git. Default configuration files should contain only production-ready channel definitions.
| "name": "test test", | |
| "type": "fluorescence", | |
| "emission_filter_position": 3, | |
| "display_color": "#FFFFFF", | |
| "enabled": true, | |
| "numeric_channel": 3, | |
| "illumination_source": null, | |
| "ex_wavelength": null | |
| }, | |
| { |
| "numeric_channel": 3, | ||
| "illumination_source": 0, | ||
| "ex_wavelength": null |
There was a problem hiding this comment.
LED matrix channel definitions should not have a numeric_channel field. According to the model validation in ChannelDefinition, LED matrix channels use illumination_source directly and should have numeric_channel set to null. However, this entry has numeric_channel set to 3, which is inconsistent with the channel type.
| # Generate a stable ID based on channel name (using MD5 for cross-session stability) | ||
| channel_id = str(int(hashlib.md5(channel_def.name.encode()).hexdigest()[:8], 16) % 100000) |
There was a problem hiding this comment.
The MD5 hash is being used for generating channel IDs, but MD5 is not cryptographically secure. While this is not a security vulnerability in this context (since it's just for generating stable IDs), consider using a more appropriate hash function like hashlib.sha256() or a simpler deterministic ID generation approach. Using MD5 may raise flags in security scanners even though it's not being used for security purposes.
| # Generate a stable ID based on channel name (using MD5 for cross-session stability) | |
| channel_id = str(int(hashlib.md5(channel_def.name.encode()).hexdigest()[:8], 16) % 100000) | |
| # Generate a stable numeric ID based on channel name (using SHA-256 for cross-session stability) | |
| channel_id = str(int(hashlib.sha256(channel_def.name.encode()).hexdigest()[:8], 16) % 100000) |
| channel_manager=channel_manager, | ||
| laser_af_manager=laser_af_manager, | ||
| base_config_path=get_test_configuration_manager_path(), | ||
| base_config_path=get_test_configuration_mananger_path(), |
There was a problem hiding this comment.
The function call contains a typo: "mananger" should be "manager". This misspelling is inconsistent with the correct spelling used elsewhere in the codebase.
| "numeric_channel": 3, | ||
| "illumination_source": 0, | ||
| "ex_wavelength": null |
There was a problem hiding this comment.
LED matrix channel definitions should not have a numeric_channel field. According to the model validation in ChannelDefinition, LED matrix channels use illumination_source directly and should have numeric_channel set to null. However, this entry has numeric_channel set to 3, which is inconsistent with the channel type.
| microscope.camera, microscope.stage, live_controller, microscope.low_level_drivers.microcontroller | ||
| ), | ||
| channel_configuration_manager=microscope.channel_configuration_manager, | ||
| channel_configuration_mananger=microscope.channel_configuration_mananger, |
There was a problem hiding this comment.
The parameter name contains a typo: "mananger" should be "manager". This misspelling is inconsistent with the correct spelling used elsewhere in the codebase.
| channel_configuration_mananger=microscope.channel_configuration_mananger, | |
| channel_configuration_manager=microscope.channel_configuration_mananger, |
| ChannelMode, | ||
| ChannelDefinitionsConfig, | ||
| ChannelDefinition, | ||
| ChannelType, |
There was a problem hiding this comment.
Import of 'ChannelType' is not used.
| ChannelType, |
software/control/utils_config.py
Outdated
| from pydantic import BaseModel, field_validator, model_validator | ||
| from pydantic_xml import BaseXmlModel, element, attr | ||
| from typing import List, Optional | ||
| from typing import List, Optional, Dict, Literal |
There was a problem hiding this comment.
Import of 'Literal' is not used.
| from typing import List, Optional, Dict, Literal | |
| from typing import List, Optional, Dict |
- Remove test channel 'test test' from default config - Fix LED matrix channels: set numeric_channel to null (was incorrectly 3) - Use SHA-256 instead of MD5 for channel ID generation - Remove unused imports (hashlib, Literal, ChannelType) - Add channel name validation in ChannelEditorDialog._save_changes - Add _sync_all_configs_from_definitions to keep legacy XML in sync Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| autofocus_controller=autofocus_controller, | ||
| objective_store=objective_store, | ||
| channel_configuration_manager=channel_configuration_manager, | ||
| channel_configuration_mananger=channel_configuration_mananger, |
There was a problem hiding this comment.
The parameter name contains a spelling error: "mananger" should be "manager". This should match the corrected parameter name.
| self.piezo: Optional[PiezoStage] = microscope.addons.piezo_stage | ||
|
|
||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_manager | ||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger |
There was a problem hiding this comment.
The attribute reference contains a spelling error: "mananger" should be "manager". This should match the corrected attribute name.
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger | |
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_manager |
Tests cover: - ChannelType enum - NumericChannelMapping model - ChannelDefinition model with validation - ObjectiveChannelSettings model - ChannelDefinitionsConfig (save/load, enabled channels) - ChannelConfigurationManager (CRUD operations, ID stability) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Write selected configurations to a file (legacy XML format for acquisition)""" | ||
| # Generate legacy XML format for backward compatibility with downstream processing | ||
| modes = [] | ||
| for i, config in enumerate(selected_configurations): |
There was a problem hiding this comment.
The loop variable i is assigned but never used in the loop body. Since it's not needed, consider removing it from the enumerate call and just iterate directly over selected_configurations.
| for i, config in enumerate(selected_configurations): | |
| for config in selected_configurations: |
| # First check if using new format | ||
| if self.channel_definitions: | ||
| for ch in self.channel_definitions.channels: | ||
| ch_id = str(int(hashlib.sha256(ch.name.encode()).hexdigest()[:8], 16) % 100000) |
There was a problem hiding this comment.
The channel ID generation uses modulo 100000 on a hash, which could potentially lead to collisions if many channels have hash values that collide after the modulo operation. This should match the logic in _build_channel_mode. Consider using a larger modulo value or the full hash prefix to reduce collision probability.
| ch_id = str(int(hashlib.sha256(ch.name.encode()).hexdigest()[:8], 16) % 100000) | |
| ch_id = hashlib.sha256(ch.name.encode()).hexdigest()[:8] |
| ObjectiveChannelSettings, | ||
| ChannelDefinitionsConfig, | ||
| ) | ||
| from control.core.channel_configuration_mananger import ( |
There was a problem hiding this comment.
The word "manager" is misspelled as "mananger" in the import statement. This typo should be corrected to maintain proper spelling.
| from control.core.channel_configuration_mananger import ( | |
| from control.core.channel_configuration_manager import ( |
| illumination_source = channel_def.illumination_source or 0 | ||
|
|
||
| # Generate a stable ID based on channel name (using SHA-256 for cross-session stability) | ||
| channel_id = str(int(hashlib.sha256(channel_def.name.encode()).hexdigest()[:8], 16) % 100000) |
There was a problem hiding this comment.
The channel ID generation uses modulo 100000 on a hash, which could potentially lead to collisions if many channels have hash values that collide after the modulo operation. Consider using a larger modulo value or the full hash prefix to reduce collision probability, especially as the system is designed to support custom channel names.
| channel_id = str(int(hashlib.sha256(channel_def.name.encode()).hexdigest()[:8], 16) % 100000) | |
| channel_id = str(int(hashlib.sha256(channel_def.name.encode("utf-8")).hexdigest()[:8], 16)) |
| microscope.camera, microscope.stage, live_controller, microscope.low_level_drivers.microcontroller | ||
| ), | ||
| channel_configuration_manager=microscope.channel_configuration_manager, | ||
| channel_configuration_mananger=microscope.channel_configuration_mananger, |
There was a problem hiding this comment.
The word "manager" is consistently misspelled as "mananger" in the parameter name. This typo should be corrected to maintain proper spelling.
| autofocus_controller=autofocus_controller, | ||
| objective_store=objective_store, | ||
| channel_configuration_manager=channel_configuration_manager, | ||
| channel_configuration_mananger=channel_configuration_mananger, |
There was a problem hiding this comment.
The word "manager" is misspelled as "mananger" in the parameter name. This typo should be corrected to maintain proper spelling and consistency throughout the codebase.
| """Tests for the channel configuration system.""" | ||
|
|
||
| import json | ||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
| import os | ||
| import tempfile | ||
| from pathlib import Path | ||
| from unittest.mock import patch, MagicMock |
There was a problem hiding this comment.
Import of 'patch' is not used.
Import of 'MagicMock' is not used.
| from unittest.mock import patch, MagicMock |
| ) | ||
| from control.core.channel_configuration_mananger import ( | ||
| ChannelConfigurationManager, | ||
| ConfigType, |
There was a problem hiding this comment.
Import of 'ConfigType' is not used.
| ConfigType, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ObjectiveChannelSettings, | ||
| ChannelDefinitionsConfig, | ||
| ) | ||
| from control.core.channel_configuration_mananger import ( |
There was a problem hiding this comment.
Typo in import statement: "mananger" should be "manager"
| from control.core.channel_configuration_mananger import ( | |
| from control.core.channel_configuration_manager import ( |
| @@ -1,8 +1,16 @@ | |||
| from enum import Enum | |||
There was a problem hiding this comment.
Typo in filename: "mananger" should be "manager". The file is named "channel_configuration_mananger.py" but should be "channel_configuration_manager.py"
| self.configuration_mananger: ConfigurationManager = ConfigurationManager( | ||
| self.channel_configuration_mananger, self.laser_af_settings_manager |
There was a problem hiding this comment.
Typo in parameter reference: "mananger" should be "manager"
| self.configuration_mananger: ConfigurationManager = ConfigurationManager( | |
| self.channel_configuration_mananger, self.laser_af_settings_manager | |
| channel_configuration_manager = self.channel_configuration_mananger | |
| self.configuration_mananger: ConfigurationManager = ConfigurationManager( | |
| channel_configuration_manager, self.laser_af_settings_manager |
| autofocus_controller: AutoFocusController, | ||
| objective_store: ObjectiveStore, | ||
| channel_configuration_manager: ChannelConfigurationManager, | ||
| channel_configuration_mananger: ChannelConfigurationManager, |
There was a problem hiding this comment.
Typo in parameter name: "mananger" should be "manager"
| self.laserAutoFocusController: LaserAutofocusController = laser_autofocus_controller | ||
| self.objectiveStore: ObjectiveStore = objective_store | ||
| self.channelConfigurationManager: ChannelConfigurationManager = channel_configuration_manager | ||
| self.channelConfigurationManager: ChannelConfigurationManager = channel_configuration_mananger |
There was a problem hiding this comment.
Typo in parameter reference: "mananger" should be "manager"
| self.channelConfigurationManager: ChannelConfigurationManager = channel_configuration_mananger | |
| channel_configuration_manager = channel_configuration_mananger | |
| self.channelConfigurationManager: ChannelConfigurationManager = channel_configuration_manager |
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger | ||
| self.laserAFSettingManager: LaserAFSettingManager = microscope.laser_af_settings_manager | ||
| self.configurationManager: ConfigurationManager = microscope.configuration_manager | ||
| self.configurationManager: ConfigurationManager = microscope.configuration_mananger |
There was a problem hiding this comment.
Typo in attribute reference: "mananger" should be "manager"
| self.piezo: Optional[PiezoStage] = microscope.addons.piezo_stage | ||
|
|
||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_manager | ||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger |
There was a problem hiding this comment.
Typo in attribute reference: "mananger" should be "manager"
|
|
||
|
|
||
| def get_test_configuration_manager_path() -> pathlib.Path: | ||
| def get_test_configuration_mananger_path() -> pathlib.Path: |
There was a problem hiding this comment.
Typo in function name: "mananger" should be "manager"
| channel_manager=channel_manager, | ||
| laser_af_manager=laser_af_manager, | ||
| base_config_path=get_test_configuration_manager_path(), | ||
| base_config_path=get_test_configuration_mananger_path(), |
There was a problem hiding this comment.
Typo in function call: "mananger" should be "manager"
| ConfigType, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Import of 'ConfigType' is not used.
| ConfigType, | |
| ) | |
| ) |
- configuration_manager -> configuration_mananger (match existing typo) - channel_configuration_manager -> channel_configuration_mananger Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
XML is no longer written to acquisition_configurations/{profile}/{objective}/
on every exposure/gain/intensity slider change. XML is only written at
acquisition start via write_configuration_selected() to the experiment folder.
This reduces unnecessary file I/O and simplifies the config flow:
- JSON is the source of truth for channel settings
- XML is only generated for acquisition records
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Migrates all existing XML configs to JSON format at app startup, not just when a profile/objective is loaded. This ensures all profiles are migrated on first run after update. - Add migrate_all_profiles() method to ChannelConfigurationManager - Call migration at Microscope initialization - Idempotent: skips if JSON already exists Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
When a channel is removed from channel_definitions.json, also remove its settings from all channel_settings.json files across all profiles and objectives. - Add base_config_path parameter to remove_channel_definition() - Add _cleanup_orphaned_settings() helper method - Update widgets.py to pass acquisition_configurations path Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1. Add ACQUISITION_CONFIGURATIONS_PATH constant to _def.py 2. Remove dead code (_save_xml_config, _sync_all_configs_from_definitions) 3. Add error handling to bulk migration with try/finally for config_root 4. Fix _migrate_from_xml_if_needed to initialize objective_settings dict 5. Add migration complete marker file (.migration_complete) to skip scanning 6. Add .migration_complete to .gitignore 7. Add 4 new tests for migration and cleanup functions (36 total) 8. Add is_dir() checks in test iterations Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Add ConfocalOverrides model for per-channel confocal-specific settings - ObjectiveChannelSettings now supports optional confocal overrides that only store values that differ from widefield defaults - Sync confocal mode from hardware at Microscope initialization (works in both GUI and headless modes) - Save confocal_mode to "acquisition parameters.json" for reproducibility - Add Microscope.set_confocal_mode() and is_confocal_mode() for headless scripts - Add get_confocal_mode() to spinning disk widgets - Add 12 new tests for confocal functionality 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add confocal/widefield mode overview section - Document channel_settings.json confocal override format - Add Settings Resolution Flow diagram for confocal mode - Document headless mode API (set_confocal_mode, is_confocal_mode) - Document acquisition metadata (confocal_mode in parameters.json) - Add troubleshooting entries for confocal issues 🤖 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 14 out of 15 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as e: | ||
| self._log.warning(f"Failed to migrate {profile_dir.name}/{objective}: {e}") |
There was a problem hiding this comment.
The error handling logs a warning but continues iterating, which is good. However, the specific exception being caught would be helpful for debugging. Consider logging the exception type and message to provide more context about what went wrong during migration.
software/control/widgets.py
Outdated
| color_btn = QPushButton() | ||
| color_btn.setStyleSheet(f"background-color: {channel.display_color};") | ||
| color_btn.setProperty("color", channel.display_color) | ||
| color_btn.clicked.connect(lambda checked, r=row: self._pick_color(r)) |
There was a problem hiding this comment.
The lambda captures the row variable in a loop without using a default parameter, which is a common Python pitfall. While the code uses r=row as a default parameter (which is correct), the checked parameter is unused and could be confusing. Consider removing the unused checked parameter for clarity.
| color_btn.clicked.connect(lambda checked, r=row: self._pick_color(r)) | |
| color_btn.clicked.connect(lambda _checked, r=row: self._pick_color(r)) |
| @model_validator(mode="after") | ||
| def validate_channel_type_fields(self): | ||
| """Validate that required fields are set based on channel type""" | ||
| if self.type == ChannelType.FLUORESCENCE and self.numeric_channel is None: | ||
| raise ValueError(f"Fluorescence channel '{self.name}' must have numeric_channel set") | ||
| if self.type == ChannelType.LED_MATRIX and self.illumination_source is None: | ||
| raise ValueError(f"LED matrix channel '{self.name}' must have illumination_source set") | ||
| return self | ||
|
|
There was a problem hiding this comment.
The model_validator runs "after" validation but raises a ValueError if validation fails. This error message won't be properly formatted by Pydantic. Consider using field_validator or custom validator methods to provide better error messages that integrate with Pydantic's validation error formatting.
| @model_validator(mode="after") | |
| def validate_channel_type_fields(self): | |
| """Validate that required fields are set based on channel type""" | |
| if self.type == ChannelType.FLUORESCENCE and self.numeric_channel is None: | |
| raise ValueError(f"Fluorescence channel '{self.name}' must have numeric_channel set") | |
| if self.type == ChannelType.LED_MATRIX and self.illumination_source is None: | |
| raise ValueError(f"LED matrix channel '{self.name}' must have illumination_source set") | |
| return self | |
| @field_validator("numeric_channel") | |
| @classmethod | |
| def validate_numeric_channel(cls, v, info): | |
| """Ensure fluorescence channels have a numeric_channel set""" | |
| channel_type = info.data.get("type") | |
| if channel_type == ChannelType.FLUORESCENCE and v is None: | |
| name = info.data.get("name", "<unknown>") | |
| raise ValueError(f"Fluorescence channel '{name}' must have numeric_channel set") | |
| return v | |
| @field_validator("illumination_source") | |
| @classmethod | |
| def validate_illumination_source(cls, v, info): | |
| """Ensure LED matrix channels have an illumination_source set""" | |
| channel_type = info.data.get("type") | |
| if channel_type == ChannelType.LED_MATRIX and v is None: | |
| name = info.data.get("name", "<unknown>") | |
| raise ValueError(f"LED matrix channel '{name}' must have illumination_source set") | |
| return v |
software/control/utils_config.py
Outdated
| if mapping: | ||
| return mapping.illumination_source | ||
| raise ValueError( | ||
| f"Fluorescence channel '{self.name}' has no mapping for numeric_channel {self.numeric_channel}" |
There was a problem hiding this comment.
The get_illumination_source method raises ValueError when mapping is missing, but the error message doesn't suggest how to fix the issue. Consider providing a more helpful error message that suggests checking the numeric_channel_mapping configuration or provides guidance on how to add the missing mapping.
| f"Fluorescence channel '{self.name}' has no mapping for numeric_channel {self.numeric_channel}" | |
| f"Fluorescence channel '{self.name}' has no numeric_channel_mapping entry " | |
| f"for numeric_channel {self.numeric_channel}. " | |
| "Check your numeric_channel_mapping configuration and add a mapping for this channel." |
|
|
||
| controller.set_microscope_mode( | ||
| microscope.configuration_manager.channel_manager.get_configurations(objective=starting_objective)[0] | ||
| microscope.configuration_mananger.channel_manager.get_configurations(objective=starting_objective)[0] |
There was a problem hiding this comment.
The variable name uses "mananger" (typo) instead of "manager". While the PR description mentions this typo is "preserved for compatibility", this appears to be introducing the typo in test code rather than preserving existing usage. Consider using the correct spelling "configuration_manager" unless there's a specific technical reason to maintain the typo.
| if name in existing_names: | ||
| QMessageBox.warning(self, "Validation Error", f"Channel '{name}' already exists.") | ||
| return | ||
|
|
There was a problem hiding this comment.
The validation only checks for empty names and duplicates within the dialog, but doesn't validate against other constraints like special characters or length limits. Consider adding validation for channel name format (e.g., no special characters that could cause filesystem issues) since channel names are used in file operations.
| # Additional validation: enforce safe format and length for filesystem use | |
| max_length = 64 | |
| if len(name) > max_length: | |
| QMessageBox.warning( | |
| self, | |
| "Validation Error", | |
| f"Channel name is too long (maximum {max_length} characters).", | |
| ) | |
| return | |
| # Disallow characters that are typically invalid or dangerous in filenames | |
| invalid_chars = '<>:"/\\|?*' | |
| # Also disallow path separators specific to this OS | |
| separators = {os.sep} | |
| if os.altsep: | |
| separators.add(os.altsep) | |
| if any(ch in invalid_chars for ch in name) or any(sep in name for sep in separators): | |
| QMessageBox.warning( | |
| self, | |
| "Validation Error", | |
| "Channel name contains invalid characters. " | |
| "Please avoid characters like <>:\"/\\|?* and path separators.", | |
| ) | |
| return |
software/control/microscope.py
Outdated
| def _sync_confocal_mode_from_hardware(self) -> None: | ||
| """Sync confocal mode state from spinning disk hardware. | ||
| Queries the actual hardware state (XLight disk position or Dragonfly modality) | ||
| and updates the channel configuration manager accordingly. | ||
| This ensures correct channel settings are used in both GUI and headless modes. | ||
| """ | ||
| confocal_mode = False | ||
|
|
||
| if self.addons.dragonfly is not None: | ||
| try: | ||
| modality = self.addons.dragonfly.get_modality() | ||
| confocal_mode = modality == "CONFOCAL" if modality else False | ||
| except Exception as e: | ||
| self._log.warning(f"Could not query Dragonfly modality: {e}") | ||
| elif self.addons.xlight is not None: | ||
| try: | ||
| # XLight returns 0 for widefield, 1 for confocal | ||
| disk_position = self.addons.xlight.get_disk_position() | ||
| confocal_mode = bool(disk_position) | ||
| except Exception as e: | ||
| self._log.warning(f"Could not query XLight disk position: {e}") | ||
|
|
||
| self.channel_configuration_mananger.sync_confocal_mode_from_hardware(confocal_mode) |
There was a problem hiding this comment.
The method silently swallows exceptions from hardware queries and logs a warning, which is reasonable. However, in case of failure, confocal_mode defaults to False without indicating to the caller that the sync failed. Consider returning a boolean or raising an exception if hardware query fails, so the caller knows whether the sync was successful.
| def migrate_all_profiles(self, base_config_path: Path) -> None: | ||
| """Migrate all profiles and objectives from XML to JSON at once. | ||
| Should be called once at app startup to ensure all existing | ||
| XML configs are migrated to the new JSON format. Uses a marker | ||
| file to skip scanning on subsequent runs. | ||
| """ | ||
| if not base_config_path.exists(): | ||
| return | ||
|
|
||
| # Check for migration complete marker | ||
| marker_file = base_config_path / ".migration_complete" | ||
| if marker_file.exists(): | ||
| return | ||
|
|
||
| migrated_any = False | ||
| for profile_dir in base_config_path.iterdir(): | ||
| if not profile_dir.is_dir(): | ||
| continue | ||
|
|
||
| for objective_dir in profile_dir.iterdir(): | ||
| if not objective_dir.is_dir(): | ||
| continue | ||
|
|
||
| objective = objective_dir.name | ||
| json_file = objective_dir / "channel_settings.json" | ||
| xml_file = objective_dir / "channel_configurations.xml" | ||
|
|
||
| # Only migrate if JSON doesn't exist but XML does | ||
| if not json_file.exists() and xml_file.exists(): | ||
| old_root = self.config_root | ||
| try: | ||
| self._log.info(f"Migrating {profile_dir.name}/{objective}") | ||
| self.config_root = profile_dir | ||
| self._migrate_from_xml_if_needed(objective) | ||
| migrated_any = True | ||
| except Exception as e: | ||
| self._log.warning(f"Failed to migrate {profile_dir.name}/{objective}: {e}") | ||
| finally: | ||
| self.config_root = old_root | ||
|
|
||
| # Create marker file to skip scanning on future runs | ||
| try: | ||
| marker_file.touch() | ||
| if migrated_any: | ||
| self._log.info("Migration complete, marker file created") | ||
| except Exception as e: | ||
| self._log.warning(f"Failed to create migration marker: {e}") |
There was a problem hiding this comment.
The migration logic sets a marker file to skip future migrations, but if a user manually adds new profiles/objectives after the initial migration, their XML files won't be migrated. Consider either documenting this limitation or implementing a per-profile marker approach to handle incremental migrations.
| self.save_configurations(objective) | ||
|
|
||
| def get_channel_configurations_for_objective(self, objective: str) -> List[ChannelMode]: | ||
| """Get Configuration objects for current active type (alias for get_configurations)""" |
There was a problem hiding this comment.
The method name is inconsistent with typical getter conventions. It returns a list of ChannelMode objects but is named "get_channel_configurations_for_objective". Consider renaming this method to match the primary method "get_configurations" or updating its docstring to explain why this alias exists.
| """Get Configuration objects for current active type (alias for get_configurations)""" | |
| """Backward-compatible alias for :meth:`get_configurations`. | |
| This method exists to support legacy code that expects the | |
| name ``get_channel_configurations_for_objective``. New code | |
| should call :meth:`get_configurations` directly. The behavior | |
| is identical and simply forwards to that method. | |
| """ |
| # Generate a stable ID based on channel name (using SHA-256 for cross-session stability) | ||
| channel_id = str(int(hashlib.sha256(channel_def.name.encode()).hexdigest()[:8], 16) % 100000) |
There was a problem hiding this comment.
The channel ID generation uses SHA-256 hash but only takes the first 8 hex characters and then performs modulo 100000. This creates a significant risk of ID collisions. For a better approach, consider using the full hash or a UUID to ensure uniqueness across all channels.
| # Generate a stable ID based on channel name (using SHA-256 for cross-session stability) | |
| channel_id = str(int(hashlib.sha256(channel_def.name.encode()).hexdigest()[:8], 16) % 100000) | |
| # Generate a stable ID based on channel name using full SHA-256 hex digest for cross-session stability | |
| channel_id = hashlib.sha256(channel_def.name.encode()).hexdigest() |
- Fix lambda parameter naming (checked -> _checked) in widgets.py - Include exception type in migration error logging - Improve error message for missing numeric_channel_mapping - Add channel name validation for length and special characters - Return success boolean from _sync_confocal_mode_from_hardware - Add docstring explaining get_channel_configurations_for_objective alias - Use full SHA-256 hash for channel IDs to avoid collisions 🤖 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 14 out of 15 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_illumination_source(self, numeric_channel_mapping: Dict[str, NumericChannelMapping]) -> int: | ||
| """Get the illumination source for this channel""" | ||
| if self.type == ChannelType.LED_MATRIX: | ||
| if self.illumination_source is None: | ||
| raise ValueError(f"LED matrix channel '{self.name}' has no illumination_source") | ||
| return self.illumination_source | ||
| else: | ||
| # Fluorescence: look up from numeric channel mapping | ||
| mapping = numeric_channel_mapping.get(str(self.numeric_channel)) | ||
| if mapping: | ||
| return mapping.illumination_source | ||
| raise ValueError( | ||
| f"Fluorescence channel '{self.name}' has no numeric_channel_mapping entry " | ||
| f"for numeric_channel {self.numeric_channel}. " | ||
| f"Check your numeric_channel_mapping configuration and add a mapping for this channel." | ||
| ) |
There was a problem hiding this comment.
The get_illumination_source method will raise a ValueError if the numeric_channel_mapping doesn't contain the required entry. This could crash the application during normal operation when configurations are being loaded or displayed. The error should be caught at a higher level or the method should return a sensible default value (with logging) to prevent crashes, especially since this is called when building channel modes for display in the UI.
| import shutil | ||
|
|
||
| shutil.copy(default_file, user_file) |
There was a problem hiding this comment.
The shutil module is imported inside the function body (line 78). While this works, it's not following Python best practices. Module imports should typically be at the top of the file unless there's a specific reason for lazy loading (e.g., performance, circular imports). Consider moving the import to the top of the file with other imports.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add model validator to ChannelDefinitionsConfig that validates all fluorescence channels have valid numeric_channel mappings. This catches configuration errors at startup rather than during use, preventing crashes when building channel modes for display. - Invalid configs now fail fast with clear error messages - Added tests for validation behavior 🤖 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 14 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| QMessageBox.warning( | ||
| self, | ||
| "Validation Error", | ||
| "Channel name contains invalid characters. " 'Avoid: < > : " / \\ | ? *', |
There was a problem hiding this comment.
The error message shows escaped backslashes which will display incorrectly. The message should use a raw string (r"...") or escape the backslashes properly. Currently it will display as "Avoid: < > : " / \ | ? *" instead of showing the actual backslash characters.
Document startup validation for numeric channel mappings and channel name constraints (length, invalid characters). 🤖 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 14 out of 15 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: str | ||
| type: ChannelType | ||
| emission_filter_position: int = 1 | ||
| display_color: str = "#FFFFFF" | ||
| enabled: bool = True | ||
| # For fluorescence channels: maps to numeric channel (1-N) | ||
| numeric_channel: Optional[int] = None | ||
| # For LED matrix channels: direct illumination source | ||
| illumination_source: Optional[int] = None | ||
| # Excitation wavelength (for fluorescence, derived from numeric_channel_mapping) | ||
| ex_wavelength: Optional[int] = None |
There was a problem hiding this comment.
The validation for channel name constraints (max 64 characters, no special characters) is only performed in the UI dialog but not in the underlying data model or API methods like add_channel_definition. This means programmatic access or direct JSON file edits could bypass validation. Consider adding these validators to the ChannelDefinition Pydantic model using field_validator to ensure constraints are enforced at all entry points.
| base_settings = self.objective_settings.get(objective, {}).get(channel_def.name, ObjectiveChannelSettings()) | ||
|
|
There was a problem hiding this comment.
The method generates a new ObjectiveChannelSettings with default values when settings don't exist for a channel, but these defaults are not persisted until the user makes a change. This could lead to confusion where the UI shows one value but the saved file doesn't contain it. Consider explicitly saving the defaults when they're generated, or documenting this lazy-initialization behavior more clearly.
| base_settings = self.objective_settings.get(objective, {}).get(channel_def.name, ObjectiveChannelSettings()) | |
| objective_settings_for_objective = self.objective_settings.get(objective) | |
| if objective_settings_for_objective is None: | |
| objective_settings_for_objective = {} | |
| self.objective_settings[objective] = objective_settings_for_objective | |
| base_settings = objective_settings_for_objective.get(channel_def.name) | |
| if base_settings is None: | |
| base_settings = ObjectiveChannelSettings() | |
| objective_settings_for_objective[channel_def.name] = base_settings |
|
|
||
| def get_channel_configuration_by_name(self, objective: str, name: str) -> ChannelMode: | ||
| def get_channel_configuration_by_name(self, objective: str, name: str) -> Optional[ChannelMode]: | ||
| """Get Configuration object by name""" |
There was a problem hiding this comment.
The method returns None when the channel is not found, but the return type annotation indicates Optional[ChannelMode]. While this is correct, callers should check for None before using the result. Consider documenting this behavior in a docstring or raising a specific exception (e.g., ChannelNotFoundError) to make error handling more explicit.
| """Get Configuration object by name""" | |
| """Get a channel configuration for an objective by its name. | |
| Args: | |
| objective: The objective name (e.g., "10x", "20x"). | |
| name: The name of the channel configuration to look up. | |
| Returns: | |
| The matching :class:`ChannelMode` if found, otherwise ``None``. | |
| Note: | |
| Callers must handle the case where this method returns ``None`` | |
| when no configuration with the given name exists for the | |
| specified objective. | |
| """ |
software/control/utils_config.py
Outdated
| except (IOError, json.JSONDecodeError) as e: | ||
| raise IOError(f"Failed to load channel definitions from {path}: {e}") |
There was a problem hiding this comment.
When loading fails due to IOError or JSONDecodeError, the error message includes the exception but doesn't provide actionable guidance for users. Consider catching specific error types and providing more helpful error messages (e.g., "File is corrupted, delete it to regenerate defaults" or "File is missing, it will be created automatically").
| except (IOError, json.JSONDecodeError) as e: | |
| raise IOError(f"Failed to load channel definitions from {path}: {e}") | |
| except FileNotFoundError as e: | |
| raise IOError( | |
| f"Channel definitions file not found at {path}. " | |
| "If this is a new installation, generate a default configuration " | |
| "using ChannelDefinitionsConfig.generate_default() and save it to this location. " | |
| f"Original error: {e}" | |
| ) | |
| except json.JSONDecodeError as e: | |
| raise IOError( | |
| f"Channel definitions file at {path} is not valid JSON and may be corrupted. " | |
| "Delete or replace the file so a new default configuration can be generated, " | |
| "then restart the application. " | |
| f"Original error: {e}" | |
| ) | |
| except PermissionError as e: | |
| raise IOError( | |
| f"Insufficient permissions to read channel definitions from {path}. " | |
| "Adjust the file permissions or run the application with appropriate access rights. " | |
| f"Original error: {e}" | |
| ) | |
| except OSError as e: | |
| raise IOError( | |
| f"Failed to load channel definitions from {path} due to an unexpected I/O error. " | |
| f"Original error: {e}" | |
| ) |
|
|
||
|
|
||
| def get_test_configuration_manager_path() -> pathlib.Path: | ||
| def get_test_configuration_mananger_path() -> pathlib.Path: |
There was a problem hiding this comment.
The function name contains the typo "mananger" instead of "manager". This creates a confusing API surface. Consider keeping both versions temporarily with proper deprecation warnings, or better yet, fixing the typo consistently across the codebase rather than propagating it.
| # TODO: USE OBJECTIVE STORE DATA | ||
| acquisition_parameters["sensor_pixel_size_um"] = self.camera.get_pixel_size_binned_um() | ||
| acquisition_parameters["tube_lens_mm"] = control._def.TUBE_LENS_MM | ||
| acquisition_parameters["confocal_mode"] = self.channelConfigurationManager.is_confocal_mode() |
There was a problem hiding this comment.
The confocal mode state is saved to acquisition parameters JSON, but there's no corresponding loading mechanism shown. When loading historical acquisition data, users won't be able to automatically restore the confocal mode state that was used. Consider adding a method to load and restore this state from saved acquisition parameters.
software/control/widgets.py
Outdated
| self.channel_manager.remove_channel_definition( | ||
| name_item.text(), base_config_path=control._def.ACQUISITION_CONFIGURATIONS_PATH | ||
| ) |
There was a problem hiding this comment.
The dialog uses hardcoded paths from control._def.ACQUISITION_CONFIGURATIONS_PATH when removing channels. This creates tight coupling between the UI layer and the specific file structure. Consider passing the base_config_path through the dialog constructor or making it configurable to improve testability and flexibility.
| self.channel_manager.remove_channel_definition( | |
| name_item.text(), base_config_path=control._def.ACQUISITION_CONFIGURATIONS_PATH | |
| ) | |
| base_config_path = getattr(self.channel_manager, "base_config_path", None) | |
| if base_config_path is not None: | |
| self.channel_manager.remove_channel_definition( | |
| name_item.text(), base_config_path=base_config_path | |
| ) | |
| else: | |
| self.channel_manager.remove_channel_definition(name_item.text()) |
|
|
||
| self.channel_definitions = ChannelDefinitionsConfig.load(user_file) | ||
| self._log.info(f"Loaded channel definitions from {user_file}") | ||
|
|
There was a problem hiding this comment.
The default file channel_definitions.default.json is copied to create the user file, but if the default file is updated in a future version, existing users won't automatically receive those updates. The documentation mentions this but doesn't provide a smooth upgrade path. Consider implementing a version field in the JSON and automatic migration logic to merge new defaults while preserving user customizations.
| self.channel_definitions = ChannelDefinitionsConfig.load(user_file) | |
| self._log.info(f"Loaded channel definitions from {user_file}") | |
| # If both user and default files exist, attempt to migrate user file to include | |
| # any new defaults while preserving user customizations. | |
| if default_file.exists(): | |
| self._migrate_channel_definitions(user_file, default_file) | |
| self.channel_definitions = ChannelDefinitionsConfig.load(user_file) | |
| self._log.info(f"Loaded channel definitions from {user_file}") | |
| def _migrate_channel_definitions(self, user_file: Path, default_file: Path) -> None: | |
| """Merge new defaults into the user channel definitions file. | |
| This implements a simple, non-destructive migration strategy: | |
| - Both files are expected to be JSON objects (dicts). | |
| - A 'version' field is used to track the schema/content version. | |
| - New top-level keys from the default file are copied into the user file. | |
| - Existing user keys are not overwritten to preserve user customizations. | |
| """ | |
| try: | |
| with default_file.open("r", encoding="utf-8") as f: | |
| default_data = json.load(f) | |
| with user_file.open("r", encoding="utf-8") as f: | |
| user_data = json.load(f) | |
| except (OSError, json.JSONDecodeError) as e: | |
| self._log.warning( | |
| "Failed to load channel definition JSON for migration (%s). " | |
| "Skipping automatic migration for %s.", | |
| e, | |
| user_file, | |
| ) | |
| return | |
| if not isinstance(default_data, dict) or not isinstance(user_data, dict): | |
| # Unexpected structure; do not attempt to modify user file. | |
| self._log.warning( | |
| "Channel definition files are not JSON objects; " | |
| "skipping automatic migration for %s.", | |
| user_file, | |
| ) | |
| return | |
| default_version = default_data.get("version", 1) | |
| user_version = user_data.get("version", 1) | |
| # Merge new top-level keys from default into user, without overwriting. | |
| changed = False | |
| for key, value in default_data.items(): | |
| if key == "version": | |
| continue | |
| if key not in user_data: | |
| user_data[key] = value | |
| changed = True | |
| # Update version if default is newer or if we added new keys. | |
| if default_version != user_version or changed: | |
| user_data["version"] = default_version | |
| try: | |
| with user_file.open("w", encoding="utf-8") as f: | |
| json.dump(user_data, f, indent=2) | |
| self._log.info( | |
| "Migrated channel definitions in %s to version %s.", | |
| user_file, | |
| default_version, | |
| ) | |
| except OSError as e: | |
| self._log.warning( | |
| "Failed to write migrated channel definitions to %s (%s).", | |
| user_file, | |
| e, | |
| ) |
There was a problem hiding this comment.
We intentionally avoid automatic migration of user customizations. Users may have specific channel configurations tuned for their hardware, and auto-merging defaults could overwrite those settings unexpectedly. The current approach (manual merge when needed) gives users full control. This is documented in the "Updating Defaults" section of channel_configuration.md.
| except json.JSONDecodeError as e: | ||
| self._log.error( | ||
| f"Failed to parse JSON in {settings_file} while cleaning up settings for " | ||
| f"'{channel_name}': {e}" | ||
| ) |
There was a problem hiding this comment.
When a JSON file fails to parse during orphaned settings cleanup, only the specific file is skipped but the error is just logged. This could leave orphaned settings in place. Consider collecting and reporting all failures at the end, or providing a repair/cleanup utility for users to run when JSON files are corrupted.
software/control/gui_hcs.py
Outdated
| # Note: Initial confocal state sync is handled in Microscope.__init__ by querying | ||
| # hardware directly. No need to sync here again - the signal connection above | ||
| # will handle any subsequent toggles by the user. |
There was a problem hiding this comment.
The comment says "Initial confocal state sync is handled in Microscope.init" but this creates fragile initialization order dependencies. If this widget is initialized before the microscope finishes initialization, or if the signal connection happens before sync, there could be state mismatches. Consider making the synchronization more explicit and robust, perhaps with a post-initialization callback or explicit state validation.
Changes: - Add channel name validation to Pydantic model (length, invalid chars) - Use shared constants for name validation in both model and UI - Document lazy initialization pattern for default settings - Add detailed docstring to get_channel_configuration_by_name() - Improve JSON load error handling with specific exception types - Remove redundant inline comment in widgets.py - Add base_config_path parameter to ChannelEditorDialog for testability - Improve orphaned cleanup error reporting (collect and return errors) - Make confocal sync initialization order more explicit in comments - Add 5 new tests for channel name validation (56 total) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
hongquanli
force-pushed
the
feature/channel-configuration-refactor
branch
from
05750be to
4cfdb01
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Generate a stable ID based on channel name using full SHA-256 hex digest for uniqueness | ||
| channel_id = hashlib.sha256(channel_def.name.encode()).hexdigest() |
There was a problem hiding this comment.
The hash-based channel ID generation using SHA-256 creates very long IDs (64 hex characters). If these IDs are stored in databases, configuration files, or URLs, consider using a shorter hash (e.g., first 16 characters of SHA-256) or a different ID scheme. Long IDs can cause issues with:
- Database query performance
- Storage overhead
- Log readability
- UI display constraints
| # Generate a stable ID based on channel name using full SHA-256 hex digest for uniqueness | |
| channel_id = hashlib.sha256(channel_def.name.encode()).hexdigest() | |
| # Generate a stable, shorter ID based on channel name using first 16 chars of SHA-256 hex digest | |
| channel_id = hashlib.sha256(channel_def.name.encode()).hexdigest()[:16] |
| except Exception as e: | ||
| error_msg = f"{settings_file}: {type(e).__name__}: {e}" | ||
| errors.append(error_msg) | ||
| self._log.warning(f"Failed to clean up {settings_file}: {type(e).__name__}: {e}") |
There was a problem hiding this comment.
Similar to the migration code, this bare exception handler could mask unexpected errors. Consider catching specific exceptions (json.JSONDecodeError, PermissionError, IOError) separately and using self._log.exception() to capture full stack traces for debugging. The current approach loses valuable debugging information when unexpected errors occur.
| except Exception as e: | |
| error_msg = f"{settings_file}: {type(e).__name__}: {e}" | |
| errors.append(error_msg) | |
| self._log.warning(f"Failed to clean up {settings_file}: {type(e).__name__}: {e}") | |
| except IOError as e: | |
| error_msg = f"{settings_file}: I/O error: {e}" | |
| errors.append(error_msg) | |
| self._log.error(f"I/O error accessing {settings_file}: {e}") | |
| except Exception as e: | |
| error_msg = f"{settings_file}: {type(e).__name__}: {e}" | |
| errors.append(error_msg) | |
| self._log.exception( | |
| f"Unexpected error while cleaning up {settings_file}: {type(e).__name__}: {e}" | |
| ) |
| # Generate a stable ID based on channel name using full SHA-256 hex digest for uniqueness | ||
| channel_id = hashlib.sha256(channel_def.name.encode()).hexdigest() |
There was a problem hiding this comment.
The channel ID is computed from the channel name using a hash. This means renaming a channel will change its ID, which could break references in saved acquisition configurations. The documentation mentions this on line 351, but the code doesn't prevent or warn about this breaking change.
Consider adding a unique, stable identifier that doesn't change when the channel name is edited, or implement migration logic to handle channel renames without breaking existing references.
| # Generate a stable ID based on channel name using full SHA-256 hex digest for uniqueness | |
| channel_id = hashlib.sha256(channel_def.name.encode()).hexdigest() | |
| # Prefer a stable identifier from the channel definition if available. | |
| # Fall back to a name-based hash only when no explicit ID is provided to | |
| # maintain backward compatibility with existing configurations. | |
| channel_id_attr = getattr(channel_def, "id", None) | |
| if channel_id_attr: | |
| channel_id = str(channel_id_attr) | |
| else: | |
| channel_id = hashlib.sha256(channel_def.name.encode()).hexdigest() |
| def _cleanup_orphaned_settings(self, base_config_path: Path, channel_name: str) -> List[str]: | ||
| """Remove orphaned channel settings from all profiles and objectives. | ||
| Args: | ||
| base_config_path: Path to acquisition_configurations folder | ||
| channel_name: Name of the channel to remove | ||
| Returns: | ||
| List of error messages for any files that failed to clean up. | ||
| Empty list if all cleanups succeeded. | ||
| """ |
There was a problem hiding this comment.
The method returns a list of error messages but doesn't provide guidance on how callers should handle them. The errors are logged but the function continues execution. Consider:
- Documenting the error handling strategy (fail-fast vs. best-effort)
- Adding a mechanism for callers to decide whether to abort or continue on errors
- Making the return type more structured (e.g., tuple of success count and errors)
| LED_MATRIX_B_FACTOR = 1 | ||
|
|
||
| DEFAULT_SAVING_PATH = str(Path.home()) + "/Downloads" | ||
| ACQUISITION_CONFIGURATIONS_PATH = Path("acquisition_configurations") |
There was a problem hiding this comment.
The ACQUISITION_CONFIGURATIONS_PATH is defined as a relative Path without documentation about what it's relative to. This could cause issues if the working directory changes. Consider:
- Making it an absolute path based on the project root
- Adding a comment explaining the expected working directory
- Using
Path(__file__).parent.parent / "acquisition_configurations"pattern for reliability
| ACQUISITION_CONFIGURATIONS_PATH = Path("acquisition_configurations") | |
| # Path to acquisition configurations directory, resolved relative to this file's location | |
| ACQUISITION_CONFIGURATIONS_PATH = Path(__file__).resolve().parent / "acquisition_configurations" |
- Use 16-char SHA-256 hash for channel IDs (readable, low collision risk) - Add TODO comment for future stable channel ID (UUID) to handle renames - Use _log.exception() for unexpected errors to capture stack traces - Document best-effort cleanup strategy in docstring 🤖 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 14 out of 15 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Check if currently in confocal mode.""" | ||
| return self.confocal_mode | ||
|
|
||
| def sync_confocal_mode_from_hardware(self, confocal) -> None: |
There was a problem hiding this comment.
The method sync_confocal_mode_from_hardware accepts parameter 'confocal' without a type hint. For consistency with the rest of the codebase and the docstring that mentions it accepts bool or int, add type hint: confocal: Union[bool, int].
| sync_successful = False | ||
|
|
||
| if sync_successful: | ||
| self.channel_configuration_mananger.sync_confocal_mode_from_hardware(confocal_mode) |
There was a problem hiding this comment.
Typo in method call: 'channel_configuration_mananger' should be 'channel_configuration_manager'. This typo is introduced in this change and should be corrected.
| else: | ||
| raise RuntimeError("No spinning disk hardware available") | ||
|
|
||
| self.channel_configuration_mananger.toggle_confocal_widefield(confocal) |
There was a problem hiding this comment.
Typo in method call: 'channel_configuration_mananger' should be 'channel_configuration_manager'. This typo is introduced in this change and should be corrected.
| if objective is None: | ||
| objective = self.objective_store.current_objective | ||
| channel_config = self.channel_configuration_manager.get_channel_configuration_by_name(objective, channel) | ||
| channel_config = self.channel_configuration_mananger.get_channel_configuration_by_name(objective, channel) |
There was a problem hiding this comment.
Typo in method call: 'channel_configuration_mananger' should be 'channel_configuration_manager'. This typo is introduced in this change and should be corrected.
|
|
||
| controller.set_microscope_mode( | ||
| microscope.configuration_manager.channel_manager.get_configurations(objective=starting_objective)[0] | ||
| microscope.configuration_mananger.channel_manager.get_configurations(objective=starting_objective)[0] |
There was a problem hiding this comment.
Typo in variable name: 'configuration_mananger' should be 'configuration_manager'. This typo is introduced in this change and should be corrected.
|
|
||
| self.configuration_manager: ConfigurationManager = ConfigurationManager( | ||
| self.channel_configuration_manager, self.laser_af_settings_manager | ||
| self.configuration_mananger: ConfigurationManager = ConfigurationManager( |
There was a problem hiding this comment.
Typo in attribute name: 'configuration_mananger' should be 'configuration_manager'. This typo is introduced in this change and should be corrected.
| self.configuration_mananger: ConfigurationManager = ConfigurationManager( | |
| self.configuration_manager: ConfigurationManager = ConfigurationManager( |
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger | ||
| self.laserAFSettingManager: LaserAFSettingManager = microscope.laser_af_settings_manager | ||
| self.configurationManager: ConfigurationManager = microscope.configuration_manager | ||
| self.configurationManager: ConfigurationManager = microscope.configuration_mananger |
There was a problem hiding this comment.
Typo in attribute name: 'configuration_mananger' should be 'configuration_manager'. This typo is introduced in this change and should be corrected.
| self.configurationManager: ConfigurationManager = microscope.configuration_mananger | |
| self.configurationManager: ConfigurationManager = microscope.configuration_manager |
| def toggle_confocal_widefield(self, confocal: bool) -> None: | ||
| """Toggle between confocal and widefield configurations""" | ||
| self.active_config_type = ConfigType.CONFOCAL if confocal else ConfigType.WIDEFIELD | ||
| def toggle_confocal_widefield(self, confocal) -> None: |
There was a problem hiding this comment.
The method toggle_confocal_widefield accepts parameter 'confocal' without a type hint. For consistency with the rest of the codebase and the docstring that mentions it accepts bool or int, add type hint: confocal: Union[bool, int].
Add Union[bool, int] type hint to toggle_confocal_widefield() and sync_confocal_mode_from_hardware() for consistency with docstrings. 🤖 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 14 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def update_channel_definition(self, channel_name: str, **kwargs) -> None: | ||
| """Update a channel definition""" | ||
| if not self.channel_definitions: | ||
| return | ||
|
|
There was a problem hiding this comment.
The method silently returns if channel_definitions is None. This makes debugging difficult when the method is called before the manager is properly initialized. Consider logging a warning or raising an exception to alert developers to this misconfiguration.
| def add_channel_definition(self, channel: ChannelDefinition) -> None: | ||
| """Add a new channel definition""" | ||
| if not self.channel_definitions: | ||
| return |
There was a problem hiding this comment.
The method silently returns if channel_definitions is None. This could mask initialization issues when trying to add a channel. Consider logging a warning or raising an exception to alert the caller that the operation failed.
| self.piezo: Optional[PiezoStage] = microscope.addons.piezo_stage | ||
|
|
||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_manager | ||
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger |
There was a problem hiding this comment.
The attribute assignment uses the typo channel_configuration_mananger (with an 'a' instead of 'e'). This propagates the naming inconsistency. Consider fixing this to use the correct spelling manager.
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_mananger | |
| self.channelConfigurationManager: ChannelConfigurationManager = microscope.channel_configuration_manager |
software/control/utils_config.py
Outdated
|
|
||
| # Channel name constraints (also enforced in UI, but validated here for direct JSON edits) | ||
| CHANNEL_NAME_MAX_LENGTH = 64 | ||
| CHANNEL_NAME_INVALID_CHARS = '<>:"/\\|?*\0' |
There was a problem hiding this comment.
The Unicode escape sequence \0 for null character should be written as a raw string or properly escaped. While Python accepts this, it's clearer to use a raw string: r'<>:"/\|?*\0' or double-escape: '<>:"/\\|?*\\0' to make the intent explicit.
| CHANNEL_NAME_INVALID_CHARS = '<>:"/\\|?*\0' | |
| CHANNEL_NAME_INVALID_CHARS = r'<>:"/\|?*\0' |
| microscope.camera, microscope.stage, live_controller, microscope.low_level_drivers.microcontroller | ||
| ), | ||
| channel_configuration_manager=microscope.channel_configuration_manager, | ||
| channel_configuration_mananger=microscope.channel_configuration_mananger, |
There was a problem hiding this comment.
The parameter name uses the typo channel_configuration_mananger (with an 'a' instead of 'e'). This propagates the naming inconsistency. Consider fixing this typo throughout the codebase.
| autofocus_controller: AutoFocusController, | ||
| objective_store: ObjectiveStore, | ||
| channel_configuration_manager: ChannelConfigurationManager, | ||
| channel_configuration_mananger: ChannelConfigurationManager, |
There was a problem hiding this comment.
The parameter name uses the typo channel_configuration_mananger (with an 'a' instead of 'e'). This typo appears throughout the codebase. Consider fixing this spelling to manager for consistency and maintainability.
| """ | ||
| if not self.channel_definitions: | ||
| return [] | ||
|
|
There was a problem hiding this comment.
The method silently returns an empty list if channel_definitions is None. This could mask initialization issues. Consider logging a warning when this condition occurs to help with debugging.
- Use raw string for CHANNEL_NAME_INVALID_CHARS with explicit null char - Log warning when update/add/remove_channel_definition called before init 🤖 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 14 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| channel_manager: ChannelConfigurationManager, | ||
| laser_af_manager: Optional[LaserAFSettingManager] = None, | ||
| base_config_path: Path = Path("acquisition_configurations"), | ||
| base_config_path: Path = control._def.ACQUISITION_CONFIGURATIONS_PATH, |
There was a problem hiding this comment.
The file name uses "mananger" (with an 'a') instead of "manager" (with an 'e'). This is a misspelling that should be corrected by renaming the file.
|
|
||
| class ChannelConfigurationManager: | ||
| def __init__(self): | ||
| def __init__(self, configurations_path: Optional[Path] = None): |
There was a problem hiding this comment.
The file name uses "mananger" (with an 'a') instead of "manager" (with an 'e'). This is a misspelling that should be corrected by renaming the file.
| Returns: | ||
| True if in confocal mode, False if in widefield mode. | ||
| """ | ||
| return self.channel_configuration_mananger.is_confocal_mode() |
There was a problem hiding this comment.
The variable name uses "mananger" (with an 'a') instead of "manager" (with an 'e'). This is a misspelling that should be corrected.
Fix NameError: 'control' is not defined in ChannelEditorDialog.__init__ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…objectives
New Pydantic models:
This pull request introduces a new, flexible channel configuration system for the microscope control software, with a focus on improving how fluorescence and LED matrix channels are defined, managed, and synchronized with hardware. It adds a global JSON-based channel definitions file, updates configuration management to support this, and enhances the GUI to allow users to edit and map channels directly. Additionally, it improves hardware synchronization for confocal mode and refactors variable naming for clarity.
Channel Configuration System Improvements:
channel_definitions.default.jsonfile insoftware/configurations/to define all available fluorescence and LED matrix channels, including their properties and hardware mappings..gitignoreto track channel configuration files and the new acquisition configurations directory.ACQUISITION_CONFIGURATIONS_PATHconstant for consistent configuration storage and access.channel_configuration_manager→channel_configuration_mananger, etc.), and updated all references accordingly. [1] [2] [3] [4] [5] [6] [7] [8]Hardware Synchronization and Management:
Microscopefor synchronizing confocal mode state from spinning disk hardware, enabling/disabling confocal mode, and querying the current mode. This ensures the channel configuration manager accurately reflects the hardware state, both in GUI and headless modes.GUI Enhancements:
Miscellaneous: