fix: preserve illumination_channel when applying confocal override#511
Conversation
get_effective_settings() was replacing the entire illumination_settings with the confocal override's copy. The override has illumination_channel=None (objective files don't store it), so the laser/LED identity was lost — turn_on_illumination() couldn't look up which source to activate. Now merges only intensity from the override, preserving illumination_channel from the base channel. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rewrite comment to accurately describe both merge strategies (field-level for illumination, full replacement for camera) - Add illumination_channel assertion to combined-override test - Add test for confocal_mode=True with no confocal_override Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Merge illumination: take only intensity from the override, preserving | ||
| # illumination_channel from the base (the override has it as None since | ||
| # it is only set in the base config). | ||
| # Merge camera: full replacement (all camera fields are tunable). | ||
| if self.confocal_override.illumination_settings: | ||
| merged_illumination = self.confocal_override.illumination_settings.model_copy() | ||
| merged_illumination = self.illumination_settings.model_copy( | ||
| update={"intensity": self.confocal_override.illumination_settings.intensity} | ||
| ) |
There was a problem hiding this comment.
The comment claims the confocal override "has [illumination_channel] as None", but IlluminationSettings allows a non-None illumination_channel (and some tests/fixtures set it). With the new merge logic, any explicitly provided override illumination_channel would be ignored. Either clarify that overrides must not set illumination_channel (and consider validating/enforcing that), or update the merge to use the override illumination_channel when it is explicitly non-None.
There was a problem hiding this comment.
[Claude Code] Skipped - illumination_channel is an identity field set only in general.yaml. The confocal override is always created without it (objective files and auto-create in repository.py both leave it as None). Silently ignoring a non-None value here is the correct behavior since confocal/widefield mode should never change which light source is used.
Summary
get_effective_settings()replaced the entireillumination_settingswith the confocal override's copy, which hasillumination_channel=None(objective files don't store it). This meantturn_on_illumination()couldn't look up which laser/LED to activate.intensityfrom the override into the baseillumination_settings, preservingillumination_channelfrom the base channel.test_effective_settings_preserves_illumination_channelto verify the fix.Test plan
🤖 Generated with Claude Code