Repository navigation
Add frame and full pose reset controls - #1137
SahilKumar75 wants to merge 4 commits into
Conversation
anna-teruel
left a comment
There was a problem hiding this comment.
Thanks a lot @SahilKumar75 for this PR! Being able to undo corrections is a really useful addition to the editing workflow. I tested it locally and it works very nicely 🚀 ✨
My main concern about your implementation is about how the loaded state is stored, which I've detailed in the inline comments. In summary:
- Reuse the load-time state instead of a separate snapshot.
POINTS_PROPERTIES_KEYalready holds the originalconfidence,editedflags andposition_is_nanfor every row, and it's whatnapari_layers_to_dscompares against on save. The only missing piece is the original positions, socapture_points_baselineandPOINTS_BASELINE_KEYcan be replaced by storing the loader's full Tracks array (incl. NaN rows, row-aligned withPOINTS_PROPERTIES_KEY) in the layer metadata when the points layer is created, for poses only. I'd suggest naming itPOINTS_POSITION_KEY, to pair it withPOINTS_PROPERTIES_KEY, but we can discuss that. - Rebuild the reset rows from those two keys. Positions from the new key, properties from
POINTS_PROPERTIES_KEY, skipping NaN rows. The internal columns (_factorized,position_is_nan) can be dropped with a small helper shared with the loader, rather than repeating the filter. - Derive symbols instead of storing them. Restored points get a ring if their loaded
editedflag is set, as on load; points in other frames keep their current symbol.
This gives a single source of truth for the loaded state, shared by save and reset, and avoids duplicating properties and symbols. I tried it on a branch to check it works: pr-1137-reset-from-original-position. Feel free to reuse anything from it.
A few smaller points:
- Docs: the user guide section is currently under "Load the tracked dataset". It should go in the "Edit tracked data" section added in #1126; I've left a suggested text in a comment above.
- "Reset all frames" discards all corrections from the session in one click, with no undo. A confirmation dialog would protect against accidental clicks? What do you think?
I hope I explained myself :) Happy to discuss any of this. Thanks again for the work on this! 🙏
| Note the additional bounding boxes layer that is loaded for bounding boxes datasets. For both poses and bounding boxes datasets, you can toggle the visibility of any of these layers by clicking on the eye icon. | ||
|
|
||
|
|
||
| ### Reset pose edits |
There was a problem hiding this comment.
There is currently a PR #1126 updating the user guide with the edit timeline widget. Your reset feature should be described inside that section once that PR is approved.
Based on the style used for that part of the user guide, I would suggest the following:
### Reset edits
If you want to discard your corrections, the `Edit tracked data` menu
provides two buttons:
- `Reset current frame` restores the moved or removed keypoints in the frame currently shown in the viewer. Edits in other frames are kept.
- `Reset all frames` restores all edited keypoints in the loaded dataset.
Both buttons restore the dataset to the state it was in when you
[loaded it](#load-the-tracked-dataset) into the viewer. This means
that if you [resumed an editing session](#resume-an-editing-session),
the edits already saved in the file are kept, and their keypoints are
still marked as edited.
Resetting does not change the file on disk. To keep the restored state,
[save the dataset](#save-the-edited-data) again.
Individual edits cannot be undone one at a time: you can only reset a whole frame or the whole dataset.Take this as a suggestion, we can further discussed once #1126 is merged.
| tracks_layer.color_by = color_by | ||
|
|
||
|
|
||
| def capture_points_baseline(points_layer: Points) -> None: |
There was a problem hiding this comment.
Instead of storing a separate snapshot here, could we reuse the load-time state we already keep on the layer?
POINTS_PROPERTIES_KEY already holds, for every row (incl. NaN rows), the original confidence, edited flags and position_is_nan. It's also what napari_layers_to_ds compares the live layer against on save. The only thing missing for a reset is the original positions. So my suggestion is:
- In
DataLoader._add_points_layer, for poses datasets, also store the loader's full Tracks array (self.data, incl. NaN rows) in the layer metadata, e.g. under aPOINTS_POSITION_KEY(we can discuss the name). It's row-aligned withPOINTS_PROPERTIES_KEY, since both come fromds_to_napari_layers. - In the reset function, rebuild the rows of the frame being reset (or of all frames) from those two: positions from the new key, properties from
POINTS_PROPERTIES_KEY, skipping rows whereposition_is_nanisTrue. Rows of other frames are kept from the current layers, and the merged rows are sorted as on load, as the PR already does. - Derive the symbols of restored points from their loaded
editedflag (ring if edited, default otherwise), as on load, instead of storing a copy of the symbols. Points in frames that aren't reset keep their current symbol.
The reset buttons would then check for the new key instead of POINTS_BASELINE_KEY.
I tried this on a branch to check that it works: pr-1137-reset-from-original-position. Let me know what you think of this. We can further discuss this.
Therefore, I would remove this capture_points_baseline function.
| } | ||
|
|
||
|
|
||
| def reset_points_to_baseline( |
There was a problem hiding this comment.
Perhaps this is more a personal taste, but what about changing this name to reset_edits(), I think that might be more clear.
| POINTS_LAYER_KEY, | ||
| POINTS_PROPERTIES_KEY, | ||
| TRACKS_LAYER_KEY, | ||
| capture_points_baseline, |
There was a problem hiding this comment.
If we do what I suggested, we would remove this because it would no longer exist.
Instead, we would add POINTS_POSITION_KEY (name to discuss) and points_layer_properties.
|
|
Thanks @anna-teruel, I used your branch and kept your commit intact. The loaded positions now use POINTS_POSITION_KEY alongside the existing properties, and Reset all frames asks for confirmation with Cancel as the default. The plugin suite passed locally with native macOS Qt: 362 tests. I then added two missing position cases and reran the reset module: all 12 tests passed. The repository hooks also pass. The Edit tracked data section is still in the open PR #1126. I will move the reset instructions there once it lands. |



Summary
Related to #1006.
Adds Reset current frame and Reset all frames controls for dragged and deleted pose points. Each loaded pose layer keeps a separate baseline of its positions, properties and marker symbols. Reset restores Points and Tracks together and refreshes the edit timeline.
This implementation uses the file as loaded as the baseline. Edits saved before loading remain intact. That is the policy question in my issue comment, and still needs maintainer confirmation.
The change includes documentation and tests for frame isolation, deleted rows, repeated reset, saved edit flags, marker styling, button availability, timeline updates and an export round trip with missing positions. Bounding boxes, identity swaps and a chronological undo stack are outside this change.
Validation
The original seven cases use ViewerModel and Qt widgets without an OpenGL canvas. An additional integration case exercises both reset controls through MovementMetaWidget with a real napari viewer and docked timeline.
Repository hooks pass, including Ruff, formatting, mypy, check manifest and codespell.
The earlier OpenGL crash was specific to the offscreen Qt platform. Native macOS Qt works. The full napari plugin suite passes locally on commit
0367d71: 360 passed, 97 warnings in 204.61 seconds.This includes the new docked widget integration case. These are automated GUI checks, not a manual visual inspection. No full repository suite or coverage result is claimed.
Remaining considerations
The baseline adds memory proportional to the loaded pose data and properties. This version copies them at load time rather than rereading a potentially changed file from disk.
Deleting every point reaches an empty Tracks array error before reset is called. I reproduced this directly in napari 0.9.2 without importing movement: create
Tracks(np.array([[0, 0, 1, 2], [0, 1, 3, 4]], dtype=float)), then assignnp.empty((0, 4))to its data. This raisesIndexErrorin_fast_points_lookup. The regression tests cover deleted points and completely deleted frames while another frame remains. Restoring a fully empty layer remains unresolved; this PR does not patch napari internals.