Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix track ids features ordering for unordered tracks #5320

Merged
merged 2 commits into from Nov 18, 2022

Conversation

JoOkuma
Copy link
Contributor

@JoOkuma JoOkuma commented Nov 9, 2022

Description

The tracks layer automatically adds the track_id as a feature. This functionality was broken when the provided tracks were not sorted by track_id and time.

For example, the example/tracks_2d.py colored by track_id with unsorted data.

wrong_track_ids.mp4

Note

A track layer features are still not idempotent:

layer = viewer.add_tracks(...)
layer.features   # corrected result
layer.features = layer.features  # sets ordered features
layer.features  # wrong values

because the features getter returns the ordered features while features setter expected the original ordering.
This should be fixed in another PR.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)

How has this been tested?

  • added a regression test for this use case;
  • tracks tests pass with my change.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added the tests Something related to our tests label Nov 9, 2022
@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #5320 (f939b74) into main (8a78b3c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5320      +/-   ##
==========================================
+ Coverage   89.07%   89.09%   +0.02%     
==========================================
  Files         581      583       +2     
  Lines       49233    49342     +109     
==========================================
+ Hits        43853    43960     +107     
- Misses       5380     5382       +2     
Impacted Files Coverage Δ
napari/layers/tracks/_tests/test_tracks.py 100.00% <100.00%> (ø)
napari/layers/tracks/_track_utils.py 87.50% <100.00%> (ø)
...apari/layers/image/experimental/_image_location.py 87.09% <0.00%> (-3.23%) ⬇️
napari/components/_layer_slicer.py 98.41% <0.00%> (-1.59%) ⬇️
napari/_qt/qt_main_window.py 76.04% <0.00%> (-0.52%) ⬇️
napari/layers/image/image.py 95.48% <0.00%> (-0.20%) ⬇️
napari/_tests/test_magicgui.py 96.42% <0.00%> (-0.02%) ⬇️
napari/utils/_base.py 100.00% <0.00%> (ø)
napari/view_layers.py 100.00% <0.00%> (ø)
napari/plugins/pypi.py 43.39% <0.00%> (ø)
... and 80 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @JoOkuma !

I think I just carried over the previous logic in #3730 without thinking too hard about it.
Agreed that the features setter being non-idempotent is a problem, but also agreed we can fix that elsewhere since this PR at least covers the case when features are not provided and data is unsorted (which might be fairly common). Can you add an issue for that if you haven't already? I think a simple solution might be to only reorder features when data is set, though in general it feels like a pain to keep these two in-sync.

napari/layers/tracks/_tests/test_tracks.py Outdated Show resolved Hide resolved
napari/layers/tracks/_tests/test_tracks.py Outdated Show resolved Hide resolved
@andy-sweet
Copy link
Member

@JoOkuma : the two comments were optional, so I'll merge this after 24 hours unless I hear otherwise.

@JoOkuma
Copy link
Contributor Author

JoOkuma commented Nov 17, 2022

Hi @andy-sweet, sorry for the delay.
Thanks for your comments, I accepted your naming suggestions.
I'll open an issue regarding the remaining problem.

@andy-sweet
Copy link
Member

Looks good - I'll merge this after 24 hours. The failing check is unrelated - it's our homegrown fuzzing tool via unseeded np.random.

@andy-sweet andy-sweet merged commit 76a81e0 into napari:main Nov 18, 2022
@Czaki Czaki mentioned this pull request Jun 7, 2023
@andy-sweet andy-sweet added this to the 0.4.18 milestone Jun 14, 2023
Czaki pushed a commit that referenced this pull request Jun 16, 2023
* fix track ids features ordering

* improving variable names
Czaki pushed a commit that referenced this pull request Jun 17, 2023
* fix track ids features ordering

* improving variable names
Czaki pushed a commit that referenced this pull request Jun 18, 2023
* fix track ids features ordering

* improving variable names
Czaki pushed a commit that referenced this pull request Jun 19, 2023
* fix track ids features ordering

* improving variable names
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* fix track ids features ordering

* improving variable names
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* fix track ids features ordering

* improving variable names
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* fix track ids features ordering

* improving variable names
@JoOkuma JoOkuma deleted the fix-track-ids branch March 18, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants