Fix/channel persistence on dataset switch#2
Open
hongquanli wants to merge 11 commits intomaragall:mainfrom
Open
Fix/channel persistence on dataset switch#2hongquanli wants to merge 11 commits intomaragall:mainfrom
hongquanli wants to merge 11 commits intomaragall:mainfrom
Conversation
When switching from an acquisition with multiple channels (e.g., 405, 488, 561) to one with fewer channels (e.g., 405 only), the old channel controls would persist in the viewer UI, showing phantom channels that don't exist in the new dataset. Changes: - Add _data_structure_changed() to detect when channels, dims, or LUTs changed - Update _maybe_refresh() to force full viewer rebuild when structure changes - Update load_dataset() to reset state before loading new dataset This ensures the NDV viewer is properly rebuilt when the data structure changes, rather than just swapping data in-place which leaves stale UI state. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use xarray's .sizes.get() instead of .coords.get() for cleaner channel count comparison - Add comprehensive test suite for _data_structure_changed() with 23 test cases covering: - Dimension changes - Channel count changes - Channel name changes - LUT changes - Edge cases (None, empty attrs, no channel dim) - Real-world scenarios (3ch->1ch, live acquisition) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address reviewer feedback: Extract _data_structure_changed from both test classes into a shared module-level data_structure_changed() function to eliminate code duplication and improve maintainability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add Args section to _data_structure_changed() docstring - Update comment to list all checks (dims, channels, channel names, LUTs) - Add NOTE in test helper about duplication risk and sync requirement 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove try-except from test helper - test code should fail visibly - Add test_luts_removed_returns_true for symmetry with luts_added test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Prevents resource leaks when switching between datasets by calling _close_open_handles() at the start of load_dataset(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Move comparison logic to module-level function for single source of truth - Add dtype change detection (uint8 vs uint16, float vs int) - Update tests to import from ndviewer_light (removes code duplication) - Add 4 new tests: dtype changes (3) and exception propagation (1) - Class method now delegates to module function with exception wrapper This addresses code review feedback: - Eliminates duplicate logic between tests and implementation - Adds missing dtype detection for contrast limit changes - Adds exception path test coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Public API method to navigate the viewer to a specific position along any dimension (fov, time, z, channel, etc.). - Encapsulates NDV's display_model.current_index API - Includes fallback for older NDV versions (dims.current_step) - Returns bool for success/failure - Validates dimension exists before setting This enables clean integration when used as a submodule, without callers needing to know NDV internals. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The instance property assignment had no effect - only the type property assignment works for MagicMock objects. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
maragall
pushed a commit
that referenced
this pull request
Jan 22, 2026
* Display channel names instead of numeric indices - Extract channel names from OME metadata or filenames - Store channel names in xarray attrs for later use - Add _update_channel_labels() method to update NDV's internal _lut_controllers with actual channel names - Use QTimer delay to ensure NDV initialization completes before updating labels 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments for channel name display - Keep xarray coords numeric instead of string channel names to avoid luts/coords type mismatch (addresses comments #2 and #3) - Replace fixed 500ms QTimer delay with retry mechanism that polls for _lut_controllers readiness with bounded retries (addresses comment #4) - Add documentation noting _lut_controllers is a private API that may change in future ndv versions (addresses comment #1) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments (round 2) - Fix single-TIFF path to use numeric coords instead of string channel names, consistent with OME-TIFF path (comment #1) - Add generation counter to cancel stale retry callbacks when viewer is replaced by a new dataset load (comment #2) - Add hasattr check for synchronize() method with debug logging when missing (comment #3) - Remove placeholder issue URL, keep explanation about private API (comment #4) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Add comment explaining synchronize() purpose 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments (round 4) - Fix misleading "all axes" comments to accurately describe which axes use numeric indices vs actual values (comments #3, #5) - Preserve partial channel names from OME metadata instead of discarding all when count doesn't match n_c (comment #4) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments (round 5) - Initialize _channel_label_generation in __init__ for clarity instead of relying on getattr defaults (comment #5) - Add debug logging when channel label update succeeds (comment #6) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments (round 6) - Rename 'channels' to 'channel_names' in single-TIFF path for consistency with OME-TIFF path (comment #1) - Initialize _pending_channel_label_retries in __init__ alongside _channel_label_generation for consistency (comment #3) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Add unit tests for channel name display feature Test coverage includes: - OME-TIFF channel name extraction and fallback logic - Single-TIFF channel name extraction from filenames - Retry mechanism with generation counter - Channel label update on NDV controllers - Integration tests for end-to-end behavior 25 tests, all passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments (round 7) - Clarify that numeric coordinates convention applies to both OME-TIFF and single-TIFF paths (comment #5) - Document graceful failure mode if private _lut_controllers API changes (comment #9) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments (round 8) - Change timeout log from debug to warning for user visibility (comment #2) - Add pytest install note to tests/README.md (comment #3) - Remove unused pytest import (comment #8) - Remove unused ch2 variable (comment #6) - Remove unnecessary pending_retries assignment (comment #7) - Add explanatory comments to except clauses (comments #9, #10, #11) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address PR review comments (round 9) - Fix debug log to track actual updated names instead of slicing list (handles controller gaps correctly) (comment #15) - Remove unnecessary getattr for _pending_channel_label_retries since it's now initialized in __init__ (comment #21) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix channel labels not updating during in-place refresh During live acquisition, if _try_inplace_ndv_update succeeds, the code returned early without scheduling channel label updates. This caused channel labels to show numeric indices instead of names (e.g., "DAPI", "GFP") after in-place data swaps. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix CI: black formatting and skip downsampling tests when unavailable - Apply black formatting to ndviewer_light.py and test_channel_names.py - Add pytest skipif marker to test_downsampling_wrapper.py to skip tests when Downsampling3DXarrayWrapper is not available (requires scipy) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Add scipy to CI environment for 3D downsampling tests scipy.ndimage.zoom is used by Downsampling3DXarrayWrapper for resampling large 3D volumes to fit within OpenGL texture limits. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Remove accidentally committed IDE config files - Remove .claude/agents/ directory (IDE-specific configuration) - Remove .coverage file (test artifact) - Add both to .gitignore to prevent future commits 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Replace line number references with method names in tests Line numbers drift as code evolves, making references misleading. Method names are more stable and searchable with IDE navigation. Updated: - test_channel_names.py: All inline comments now reference method names - tests/README.md: Documentation uses method names instead of line numbers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix README: "three main areas" → "five main areas" The test file has five test classes, not three. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Change synchronize() fallback log from debug to warning If synchronize() method doesn't exist on LUT controller, channel labels may not appear in the UI. A warning is more appropriate than debug level. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Use _ = for intentionally unused ET.SubElement return value Makes explicit that the return value is intentionally unused. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Extract constants and helper for channel label update retry - Add CHANNEL_LABEL_UPDATE_MAX_RETRIES (20) constant - Add CHANNEL_LABEL_UPDATE_RETRY_DELAY_MS (100) constant - Extract _initiate_channel_label_update() helper method to DRY up duplicated code between _store_data_from_loader and _set_ndv_data 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.