Visualize bounding boxes as Napari shapes layer

[DPWebster]

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

Closes #567

Adds a checkbox to toggle loading VIA-Tracks bounding box data as Napari shapes layers and functionality for loading shapes data from a bounding boxes dataset (containing heights/widths and centroid coordinates for a rectangular bounding box).

What is this PR

• Addition of a new feature

Why is this PR needed?

Allows users to visualize VIA-Tracks bounding boxes data sets as rectangular bounding boxes instead of only visualizing their centroids.

What does this PR do?

Includes functionality for loading a VIA-Tracks file as a Napari shapes layer representation (in addition to points + tracks) which can be toggled on or off via a checkbox.

References

#567

How has this PR been tested?

Pytest run to ensure no existing functionality breaks. New functionality verified by attempting to load sample data (VIA_single-crab_MOCA-crab-1.csv, VIA_multiple-crabs_5-frames_labels.csv) with checkbox ticked. Also attempted to verify existing functionality still works by loading other datasets (DLC, etc.) with and without the checkbox ticked.

Is this a breaking change?

This feature should not break existing functionality.

Does this PR require an update to the documentation?

Yes, gui.md has been updated to reflect the new functionality..

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[DPWebster]

Hi @sfmig, this is a basic implementation of the bounding boxes visualization from #567. I still need to add test coverage, documentation and the optional features of #17 and displaying areas but I wanted to get something presentable so I could ask for further input.

• While developing this I noticed that one entry from the source file will always correspond to one frame in the viewer even if the fps slider is changed or the data indicates that one entry should correspond to multiple frames (e.g. VIA_single-crab_MOCA-crab-1.csv has most of its entires correspond to 5 frames; it should have 168 frames of data, whereas our functions will load 35). I had spent some time working on a fix for this before I noticed it affected all data and not just the bounding box loading functions I was working on. Is this intended behavior or would it be useful to ensure the number of frames we load to the viewer matches up with the time column? (I see that there are some other issues related to this, so that might be something to work on after I finish up here.)

• Based on discussion in Automatically generate bounding boxes annotations from pose data #102, bounding boxes for poses datasets could be visualized as a convex hull or as a rectangle. Is there any particular preference for which, or both?

[sfmig]

Hi @DPWebster, thanks for having a go.

This looks like a good start. However, I have not done a detailed review since the PR is still a draft. Whenever you feel that it is as finished as it can be on your side, feel free to mark it as "Ready for review". Someone in the team (probably me) will review it. "Ready to review" usually means the functionality is in, along with tests, updated docs and a PR description.

In the description you mention that you plan to add two more features: visualisation for bboxes for poses datasets, and displaying of bboxes areas. I would strongly suggest dealing with these two features as two separate PRs. The main reason for this is PR size. Small PRs are faster to understand and to review, and often will get merged more quickly. It is also easier to write tests for a small chunk of work.

Re your questions:

While developing this I noticed that one entry from the source file will always correspond to one frame in the viewer even if [...] the data indicates that one entry should correspond to multiple frames (e.g. VIA_single-crab_MOCA-crab-1.csv has most of its entires correspond to 5 frames; it should have 168 frames of data, whereas our functions will load 35)

This is not a problem for us at the moment. It happens because in ds_to_napari_tracks, the time column that napari uses for the slider is "recomputed" to count number of frames based on the input data.

This was the original implementation of the widget, I believe for simplicity. We could instead use the timestamps as specified in the input movement dataset ds. This may make more sense for bboxes datasets (note the use_frame_numbers_from_file argument in the bboxes loading function). For poses, we currently refer all timepoints to the start of the data (so using the dataset timestamps or recomputing them based on the data are equivalent), but we are exploring expanding this to other formats (see #473). I opened an issue to discuss (#595), feel free to chime in with any thoughts.

For now I would recommend using the interpolated version of that dataset instead when you are trying things out. The VIA_single-crab_MOCA-crab-1.csv file is somewhat confusing due to the data being defined for one every 5 frames, but this will rarely be the case.

Re the question about convex hull / rectangles: it is usually a good idea to keep things as consistent and simple as possible, so in this case I believe that would be "rectangles". But probably better to discuss this in its relevant PR when we get to it.

I left a couple more comments in the code, although I realise it is not finished yet. Hope it helps!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6b6a4d8) to head (732a3a1).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #590   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           32        32           
  Lines         1817      1861   +44     
=========================================
+ Hits          1817      1861   +44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sfmig]

Hi @DPWebster,

This is a very nice attempt and thanks for joining the call today! I think it was good to discuss it in a group.

I left some in-line comments but have a couple of general ones.

As discussed, we decided for simplicity to always load the boxes layer if the shape data array is available in the loaded movement dataset. So we would need to remove the checkbox widget - hopefully it also simplifies things a bit with the implementation and tests.

Re the "white bboxes" issue I found, I confirm that it happens if the loaded dataset has nans in the shape array. This may happen if not all individuals are present in all frames. I added a sample dataset to GIN where this is the case (VIA_multiple-crabs_5-frames_labels_missing.csv) to help us check this "manually". However, we should also properly test this (i.e. test that data with nans is loaded with the expected colors for boxes, tracks and points). If you want to do that in this PR that would be great, but it is also fine if we raise an issue and deal with it separately.

I also found some comments and docstrings were somewhat outdated, if you could have a look that would be great.

Thanks again for the work!

[sfmig]

Hi @DPWebster, I think this is almost there!

I left some more substantial comments on the tests since this is their first review, and smaller suggestions for the rest.

Remember to mark as "resolved" any comments that you feel have been addressed and feel free to leave further comments otherwise. This makes it easier for the reviewer to parse and things are less likely to get lost. For example, I noticed the shortcuts for customising the bboxes appearance are gone in this PR (see comment here) - I actually thought that was very useful to include in the guide (it is somewhat hard to find this info in the napari docs).

Hope this helps! Thanks for the great work

[DPWebster]

I ended up deleting this admonition in the last commit as it turns out the keyboard shortcuts for shapes layers are surprisingly sparse. I originally assumed they would be similar across different layer types, but this isn't the case; shapes layers don't support deselecting shapes via hotkey, or selecting shapes across multiple layers, for example. Let me know if you think it is still useful in this state.

[sfmig]

I see! I agree it's unfortunate that it is not more consistent across layer types as you say (esp the selection across frames would be nice). But I do think it may still be useful as is, for example to color all boxes in the same color (I do this sometimes to visually compare datasets). I say we leave it for now and if we get any feedback that this bit is confusing we can remove it.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sfmig]

Great job @DPWebster, this looks ready to go!

I made a few very small changes and commented them in the PR for reference - feel free to have a look if you are curious.

Do reach out here or in zulip if you'd like to contribute further to movement, but in any case thanks for this nice piece of work!

[sfmig]

I see! I agree it's unfortunate that it is not more consistent across layer types as you say (esp the selection across frames would be nice). But I do think it may still be useful as is, for example to color all boxes in the same color (I do this sometimes to visually compare datasets). I say we leave it for now and if we get any feedback that this bit is confusing we can remove it.

[sfmig]

I changed this to use the ds_type attribute instead, to more strictly restrict it to bboxes dataset (for now)

if ds.ds_type == "bboxes":

[sfmig]

It should contain a column with the same name as
property as well as a factorized version of this column with a
name equivalent to "property+'_factorized'".

I think it's a good call to make future contributors (or future us) aware of this, but this won't cause an issue in this specific function (since we don't retrieve the data from the dataframe here) so I moved it to the notes section of the docstring instead.

[sfmig]

I parametrised this test over the two files (DLC and VIA-tracks) instead

[sfmig]

I changed a little bit this test to make it clear that we can color by any property (even tho we always color by individual now), and that we just expect to have as many colors as unique values of that property (rather than as many colors as individuals)

[DPWebster]

I'm still interested in contributing more; that said, I'm going to be especially busy with schoolwork until August so it's unlikely I'll have much time to spare until then. Something that doesn't need to be ready in the short term is probably ideal. You've mentioned supporting visualization of segmentation masks previously and there doesn't seem to be a pull request open for that yet, maybe that's a good choice?

[sfmig]

Hi @DPWebster,

That sounds great! There are a few things that are not immediately needed but could be good candidates:

• a nice follow up to this work could be implementing functionality to derive a bboxes dataset from a keypoint (poses) one (see issue #102).
• or if you'd like to continue working with napari, we could tackle this issue on showing prior markers along the trajectory (see issue #571. I had a go at it in this now-closed PR (#505) which may be a useful starting point.
• supporting loading data in which not all the individuals have the same skeletons, see issue #150. In its simplest form, the fix could be just an update to the documentation for now.

I think adding support for visualising segmentation masks in napari could also be very nice, and as you say there is not much rush for it. However, it would make sense to first support adding segmentation masks to a movement dataset, and that may require more discussion / thought (see latest thoughts here) . It may even make more sense to do that after we "unify" pose and bboxes datasets (if we go down that path, see issue #241 )