Improve images-annotation remapping logic#194
Conversation
Add strict validation and logging for frame index remapping to avoid mixing remapped and old indices. _remap_array now detects unmapped integer frame values and raises a KeyError instead of silently leaving them unchanged, and uses direct idx_map lookups. Introduce _used_time_indices helper to collect actually used frame indices from layer data (supports arrays and lists of vertex arrays). remap_layer_data_by_paths now computes used vs mapped indices, logs coverage and examples, and rejects the remap (returning an ambiguous RemapResult) if any used frame indices lack a mapping. Also add debug output of example mappings and simplify/rename ambiguous-remap handling and messages. These changes improve safety and diagnostics when remapping tracked-frame data.
Introduce user-facing notifications for remap warnings and errors using napari's show_warning/show_error and new signals (remap_warning_requested, remap_error_requested). Add _notify_remap_issue to set a concise status bar message, emit signals, and show dialogs. Handle ambiguous or failed remaps by logging, notifying the user, hiding Points layers that couldn't be safely aligned, and aborting further processing. Also notify when remaps are applied with warnings and tighten up remap-related logging for clarity.
Introduce RemapOutcome and RemapReason enums and reshape RemapResult to provide richer, explicit outcomes for remapping operations. remap.py: replace boolean flags with outcome/reason fields, add properties (applied, paths_updated), tighten remapping semantics (reject when used indices are unmapped, stricter time-column handling), support partial-row drops with diagnostics, produce clearer warnings and logging, and simplify index mapping logic. manager.py: adapt to the new RemapResult API (import enums), enhance logging messages, mark Points presentation changes only when data or paths change, differentiate hard failures vs. skipped cases to set notification level, and refine when layers are hidden or warned. Overall this improves safety, observability, and diagnostics for layer path remapping.
Add support for partial remaps when some frame indices in layer data cannot be mapped to the new image stack. remap_time_indices gained an allow_partial flag and now drops unmappable rows (reporting dropped counts and example frames) instead of outright rejecting, with clearer reasons/messages and outcome variants. _filter_and_remap_array message texts were updated and handling for no-mappable-rows was added. remap_layer_data_by_paths now accepts allow_partial and propagates it to remap_time_indices while appending warnings when partial remaps are expected. LayerLifecycleManager now enables allow_partial for Points layers and logs/notifies when a partial remap is applied, including warning details and dropped-row/frame information.
Make partial remaps more permissive and reduce noisy warnings. - manager.py: Prevent duplicate warning logs by making the "warnings + paths_updated" case exclusive: log either a generic remap-with-warnings or a specific partial-remap warning depending on the RemapOutcome. - remap.py: Remove the early NO_MAPPABLE_ROWS short-circuit so empty/partially mappable arrays can be handled (enabling permissive/partial remaps). Clarify strict vs partial behavior in the docstring. Suppress low old-path coverage warnings during allow_partial + unmapped-used cases (log at debug level instead) and adjust coverage messages to reflect the permissive path. These changes reduce false rejections and noisy warnings when performing partial remaps that drop unmappable rows.
Update remap-related tests to use the new RemapOutcome and RemapReason enums and to assert the richer result API (applied, paths_updated, mapped/dropped counts, dropped_frame_indices, warnings, messages). Rename several tests to reflect changed semantics (noop/skipped/rejected) and adjust logging expectations. Add new tests covering strict rejection of unmapped used indices, partial remap behavior (dropping rows when allow_partial=True), partial rejection when no rows are mappable, and suppression of low-coverage warnings during partial remaps. Minor cleanup of docstrings and input formatting.
There was a problem hiding this comment.
Pull request overview
This PR refines the annotation frame-index remapping pipeline by introducing explicit remap outcomes/reasons, supporting partial remaps (dropping unmappable rows), and surfacing remap problems to users via napari notifications during layer lifecycle operations.
Changes:
- Introduces
RemapOutcome/RemapReasonand upgradesRemapResultto structured outcome reporting (incl. partial remap drop diagnostics). - Updates remap logic to support strict vs. partial remapping behavior, with clearer safety checks and warnings.
- Adds lifecycle-manager user notifications and new signals for remap warnings/errors; updates tests to assert on the new result semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/napari_deeplabcut/core/remap.py |
Adds structured remap result enums + partial remap support and diagnostics; updates remap decision logic and warning behavior. |
src/napari_deeplabcut/core/layer_lifecycle/manager.py |
Emits user-facing warnings/errors and signals for remap issues; integrates new remap result semantics into layer updates. |
src/napari_deeplabcut/_tests/core/test_remap.py |
Refactors/extends tests to validate outcome/reason/partial-remap behavior and new warning semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Remap signals | ||
| remap_warning_requested = Signal(str) | ||
| remap_error_requested = Signal(str) | ||
|
|
There was a problem hiding this comment.
The new remap_warning_requested / remap_error_requested signals and _notify_remap_issue behavior don’t appear to have test coverage (no tests assert these signals or notification behavior, and existing lifecycle manager tests stub _remap_frame_indices). Consider adding unit tests that exercise _remap_frame_indices outcomes (REJECTED/SKIPPED/APPLIED_PARTIAL) and assert emitted signals and visibility changes.
| missing = [int(v) for v in values_int if int(v) not in idx_map] | ||
| if missing: | ||
| raise KeyError(f"Unmapped frame indices encountered during remap: {missing[:10]}") |
There was a problem hiding this comment.
_remap_array builds missing as a full list of every unmapped frame index. For large layers this can be very memory- and time-expensive even though the error message only shows the first 10. Consider collecting at most N examples (and optionally a count) and short-circuiting once enough are found, instead of materializing the full list.
| t = arr[:, time_col].astype(int, copy=False) | ||
| keep_mask = np.array([v in idx_map for v in t], dtype=bool) | ||
| drop_mask = ~keep_mask |
There was a problem hiding this comment.
_filter_and_remap_array computes keep_mask with a Python loop ([v in idx_map for v in t]). For large point clouds this can become a hotspot. Prefer a vectorized approach (e.g., np.isin(t, keys) and then remap via indexing or a vectorized lookup) to avoid per-element Python overhead.
| level: str, | ||
| message: str, | ||
| details: tuple[str, ...] | list[str] | None = None, | ||
| ) -> None: | ||
| """Show a user-facing remap notification.""" | ||
| details = list(details or []) | ||
| text = f"{getattr(layer, 'name', 'Layer')}: {message}" | ||
| if details: | ||
| text += "\n\n" + "\n".join(details) | ||
|
|
||
| # Keep the status bar concise. | ||
| self.viewer.status = f"{getattr(layer, 'name', 'Layer')}: {message}" | ||
|
|
||
| if level == "error": | ||
| self.remap_error_requested.emit(text) | ||
| show_error(text) | ||
| else: | ||
| self.remap_warning_requested.emit(text) | ||
| show_warning(text) |
There was a problem hiding this comment.
_notify_remap_issue takes level: str and treats any non-"error" value as a warning. That makes typos (e.g. "Error") silently downgrade severity. Consider using an Enum/Literal type (or validating level and defaulting to error) so the call sites can’t accidentally misclassify notifications.
| self._notify_remap_issue( | ||
| layer=layer, | ||
| level="warning", | ||
| message=( | ||
| "Partially aligned this annotation layer to the current image stack. " | ||
| "Rows referring to frames that could not be matched were ignored." | ||
| ), | ||
| details=[ | ||
| res.message, | ||
| f"Dropped rows: {res.dropped_row_count}", | ||
| f"Dropped frame examples: {list(res.dropped_frame_indices)}", | ||
| *res.warnings, | ||
| ], | ||
| ) |
There was a problem hiding this comment.
The if res.warnings and res.paths_updated: branch always notifies with the message "Partially aligned…" even when the outcome is APPLIED_FULL or NOOP (it only changes the logging). This produces misleading user-facing text for non-partial remaps. Consider tailoring the notification message to res.outcome (or only using this branch for APPLIED_PARTIAL) and rely on the later if res.applied and res.warnings: notification for full remaps with warnings.
| self._notify_remap_issue( | |
| layer=layer, | |
| level="warning", | |
| message=( | |
| "Partially aligned this annotation layer to the current image stack. " | |
| "Rows referring to frames that could not be matched were ignored." | |
| ), | |
| details=[ | |
| res.message, | |
| f"Dropped rows: {res.dropped_row_count}", | |
| f"Dropped frame examples: {list(res.dropped_frame_indices)}", | |
| *res.warnings, | |
| ], | |
| ) | |
| self._notify_remap_issue( | |
| layer=layer, | |
| level="warning", | |
| message=( | |
| "Partially aligned this annotation layer to the current image stack. " | |
| "Rows referring to frames that could not be matched were ignored." | |
| ), | |
| details=[ | |
| res.message, | |
| f"Dropped rows: {res.dropped_row_count}", | |
| f"Dropped frame examples: {list(res.dropped_frame_indices)}", | |
| *res.warnings, | |
| ], | |
| ) |
| if res.applied and res.warnings: | ||
| self._notify_remap_issue( | ||
| layer=layer, | ||
| level="warning", | ||
| message=("Annotations were remapped to the current image stack, but the match had issues."), |
There was a problem hiding this comment.
For APPLIED_PARTIAL, this code will trigger two user notifications: one in the if res.warnings and res.paths_updated: branch above, and another in if res.applied and res.warnings: below. This can result in duplicate warning popups/status updates for a single remap. Consider making these branches mutually exclusive (e.g., skip the second notify for APPLIED_PARTIAL, or consolidate into one notification path).
C-Achard
added
the
I/O
Scope
Refines the remapping logic in
remap_layer_data_by_pathsand related functions, introduces more explicit, structured handling of remapping outcomes and reasons.It also adds user-facing notifications for remap issues in the layer lifecycle manager.
The changes improve both the robustness of remap operations and the clarity of their results, especially in edge and partial remap cases.
The plugin now also accepts H5s with only a subset of valid/matching annotations, and will report which entries were omitted.
Automated summary
User notification improvements:
_notify_remap_issuemethod toLayerLifecycleManagerto emit signals and show user-facing warnings or errors in the napari UI when remap issues occur, improving transparency and debug. [1] [2]show_errorandshow_warningfor displaying notifications.API and code structure updates:
RemapOutcomeandRemapReasonenums, making remap result handling more explicit and maintainable. [1] [2]Test suite improvements and expanded coverage:
outcome,reason,applied,paths_updated) instead of the previouschangedflag, improving clarity and future-proofing. Added checks for user-facing messages and warnings. [1] [2] [3] [4] [5]REJECTED/REMAP_FAILED). (F5645e13L232R232)APPLIED_PARTIAL/PARTIAL_ROWS_DROPPED). (F5645e13L246R246)REJECTED/NO_MAPPABLE_ROWS). (F5645e13L273R273)REJECTED/AMBIGUOUS_MATCH).Nonedata (SKIPPED/NO_DATA). (F5645e13L273R273)