Document napari plugin

[niksirbi]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other: Documentation

Why is this PR needed?

Closes #283

What does this PR do?
This PR puts some final necessary touches before we are able to release the napari plugin into the world.

• It modifies installation instructions to account for the napari plugin
• Add napari-video and pyvideoreader to the [napari] extras dependencies. These are necessary for loading videos into as an image layer in napari.
• Adds a new page on the "User guide" documenting how to install and use the movement plugin for napari. This guide includes some warnings, to manage expectations, and some advice on how to troubleshoot video playback issues.
• To ameliorate the aforementioned playback issues, which mostly manifest when trying to "play" the video in napari at hifgh fps, I modified the default behaviour of the plugin. Previously, the plugin changed napari's playback fps setting to the same value as the loaded data (e.g. if loading data tracked at 60 fps, the plugin would automatically set napari's palyback speed to that values, so that the video would play "in real time" by default). This PR removes that feature, and instead leaves that choice entirely to the user (documented in the guide). This means that user's will get napari's default playback rate of 10 fps, and can choose to change that however they like.

References

After this PR is successfully merged into napari-dev, we will have achieved all tasks listed under the meta-issue #31.
After that, we can finally merge napari-dev into main, write a blogpost about the plugin and finally make our v0.1.0 release.

How to review this PR

The best way to review the PR would be to build the docs locally, and then follow the instructions in updated docs, i.e.:

• install movement in dev mode (this also gets you all the napari-related dependencies
• Follow "The napari plugin" user guide and see if you can reproduce my steps. You can use some of our sample data if you don't have access to your own. The datasets containing "EPM" or "Aeon" have associated videos with them, so you can do ds = sample_data.fetch_dataset(filename, with_video=True), and find the files in ~/.movement.

The reason I'm tagging many reviewers here is the following:

• I want the workflow to be tested on as may computers and OSes as possible
• @willGraham01. @sfmig and @stellaprins will soon be undertaking further napari plugin development, so I thought y'all will benefit from seeing the current (limited) status quo.
• @lochhh is a Windows user and has previously reported some issues with napari.

How has this PR been tested?

No new functionality has been added. Some tests were modified to account for the removal of the aforementioned fps functionality.

Is this a breaking change?

No.

Does this PR require an update to the documentation?

It is, for the most part, an update to documentation.

Checklist:

• The code has been tested locally
• [n/a] 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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.82%. Comparing base (5d5e5ed) to head (cdb3fcc).
Report is 11 commits behind head on napari-dev.

Additional details and impacted files

@@             Coverage Diff             @@
##           napari-dev     #393   +/-   ##
===========================================
  Coverage       99.82%   99.82%           
===========================================
  Files              18       18           
  Lines            1122     1175   +53     
===========================================
+ Hits             1120     1173   +53     
  Misses              2        2           

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

[willGraham01]

I'm able to follow the instructions and get the plugin working, all the listed features in the guide seem to work 🥳

• I couldn't load any of the SLEAP poses datasets, but I imagine that's just me trying to load the wrong type of data in? I got a ValueError: Could not find the expected dataset(s) {'df_with_missing'} in file: /home/ccaegra/.movement/data/poses/SLEAP_single-mouse_EPM.analysis.h5 when trying to do so, quickly followed by a seg-fault in the C-backend.

One thing I did realise is that we appear to have a memory leak issue when the user deletes the layers they read in. To recreate:

• Fire up napari, napari -w movement
• Load any valid dataset, EG DLC_single-mouse_EPM.predictions.h5 (as in the user-guide).
• The slider and the "play" button will allow the user to move through the frames as expected.
• Delete the imported layers using the napari layers tab.
• Load any valid dataset (even the same one) in again via the plugin.
• The slider can now be manually dragged to move through the loaded image, but pressing "play" results in a seg-fault (see below).

Valgrind's traceback goes deep into the C/C++ backend, but it looks like the fault originates from a napari object. So either we're not cleaning up our references to layers properly - which I don't think is the problem having looked at the widget's class (we aren't storing any references and are just passing the imported layer to the core napari instance to handle itself) - or (more likely) this is a genuine bug with the napari backend.

[niksirbi]

I couldn't load any of the SLEAP poses datasets, but I imagine that's just me trying to load the wrong type of data in? I got a ValueError: Could not find the expected dataset(s) {'df_with_missing'} in file: /home/ccaegra/.movement/data/poses/SLEAP_single-mouse_EPM.analysis.h5 when trying to do so, quickly followed by a seg-fault in the C-backend.

That error message indicates that you may have tried to load a file output by SLEAP, using the DeepLabCut loader (becasue "df_with_missing" is a dataset within DeepLabCut .h5 output files). Can you try again loading the same file but with source software set to SLEAP?

[niksirbi]

One thing I did realise is that we appear to have a memory leak issue when the user deletes the layers they read in. To recreate:

Yeah that's very annoying, I can reproduce it. It's likely on the napari side (see this issue), but it would be good to make sure that we are not creating this problem).

[sfmig]

Looks great! My first time using the napari plugin 🤩

All small nitpicky comments, feel free to take or leave them! Mostly about making the guide sound a bit less technical / dev-oriented.

I found some behaviour with the fps a bit odd:

• seems like it cannot be a float. But in a movement dataset we do set the fps as a float.
• it also seems like the default is set to 30fps. Would this be better as 0, or as 1 (to match the frame number in the slider)? Note that the tooltip doesn't specify units. Since it is now decoupled from the playback speed and it only affects the tooltip data, I was imagining what would someone input if they didn't know or care about the fps when just visualising the data.

[sfmig]

We could put the annotations in red to make them more visible (the "Plugin" one may be easily missed).

[niksirbi]

Red actually didn't contrast very well against the background. But I made all annotations bigger and bolder:

[willGraham01]

That error message indicates that you may have tried to load a file output by SLEAP, using the DeepLabCut loader (becasue "df_with_missing" is a dataset within DeepLabCut .h5 output files). Can you try again loading the same file but with source software set to SLEAP?

Can confirm, this was indeed the problem 😅 Data loaded OK when I switched.

[sfmig]

That error message indicates that you may have tried to load a file output by SLEAP, using the DeepLabCut loader (becasue "df_with_missing" is a dataset within DeepLabCut .h5 output files). Can you try again loading the same file but with source software set to SLEAP?

Can confirm, this was indeed the problem 😅 Data loaded OK when I switched.

Maybe we can make the error message a bit more clear? 👀 (I encountered this and was surprised because generally movement has crystal clear messages)

[niksirbi]

That error message indicates that you may have tried to load a file output by SLEAP, using the DeepLabCut loader (becasue "df_with_missing" is a dataset within DeepLabCut .h5 output files). Can you try again loading the same file but with source software set to SLEAP?

Can confirm, this was indeed the problem 😅 Data loaded OK when I switched.

Maybe we can make the error message a bit more clear? 👀 (I encountered this and was surprised because generally movement has crystal clear messages)

I agree that error message should be modified by adding something "Are you sure you've selected the correct source_software"? But I won't do this here because it concerns the io and validator moduels. I will open an issue.

Hopefully, using Sofía's updated step-by-step instructions this error will occur less frequently (or not at all if we switch to inferring the file format, and enable drag-and-dropping files (which we should!).

[willGraham01]

I agree that error message should be modified by adding something "Are you sure you've selected the correct source_software"? But I won't do this here because it concerns the io and validator moduels. I will open an issue.

You can always do a "try...except" catch in the widget code, if we know the specific errors. But then we'd likely get into the rabbit hole of needing our own custom exceptions to reliably identify errors like this, and errors where the file is genuinely faulty, for example

[niksirbi]

• seems like it cannot be a float. But in a movement dataset we do set the fps as a float.
• it also seems like the default is set to 30fps. Would this be better as 0, or as 1 (to match the frame number in the slider)? Note that the tooltip doesn't specify units. Since it is now decoupled from the playback speed and it only affects the tooltip data, I was imagining what would someone input if they didn't know or care about the fps when just visualising the data.

@sfmig I've now changed the fps spinbox to:

• have a float datatype instead of integer
• default to 1 (which effectively makes time indices equal frame indices), so that's what I also recommend in the guide now for cases where the fps is unknown or the user doesn't care.
• display an informative tooltip that explains what fps does when hovering over it.

def _create_fps_widget(self):
	"""Create a spinbox for selecting the frames per second (fps)."""
	self.fps_spinbox = QDoubleSpinBox()
	self.fps_spinbox.setObjectName("fps_spinbox")
	self.fps_spinbox.setMinimum(0.1)
	self.fps_spinbox.setMaximum(1000.0)
	self.fps_spinbox.setValue(1.0)
	self.fps_spinbox.setDecimals(2)
	# How much we increment/decrement when the user clicks the arrows
	self.fps_spinbox.setSingleStep(1)
	# Add a tooltip
	self.fps_spinbox.setToolTip(
	    "Set the frames per second of the tracking data.\n"
	    "This just affects the displayed time when hovering over a point\n"
	    "(it doesn't set the playback speed)."
	)
	self.layout().addRow("fps:", self.fps_spinbox)

[niksirbi]

One thing I did realise is that we appear to have a memory leak issue when the user deletes the layers they read in. To recreate:

• Fire up napari, napari -w movement
• Load any valid dataset, EG DLC_single-mouse_EPM.predictions.h5 (as in the user-guide).
• The slider and the "play" button will allow the user to move through the frames as expected.
• Delete the imported layers using the napari layers tab.
• Load any valid dataset (even the same one) in again via the plugin.
• The slider can now be manually dragged to move through the loaded image, but pressing "play" results in a seg-fault (see below).

@willGraham01 I actually have lots of trouble diagnosing this issue. I went through reports of similar issues, like this one but I frankly don't understand the Qt internals well enough to determine what causes this (and how to solve it).

What course of action would you propose?

[lochhh]

Thanks @niksirbi! All LGTM 🚀 and sorry for being late to the party! Only left a few minor suggestions, and as usual, feel free to take or leave.

Edit: do we want to mention movement launch?

[sfmig]

Just noticed a potential issue and wanted to check if it is only on my side. I didn't realise before.

Steps:
1- napari -w movement
2- Load the sample data file SLEAP_two-mice_octagon.analysis.h5
3- The frame slider is at frame 256, but I can see keypoints for both frame 255 and frame 256. Notice there are more keypoints per individual than expected (the corresponding movement dataset says 7 keypoints)

I also get this error message:

I am surprised I didn't notice this before, which makes me suspicious that the issue is on my end, but if other folks can check that would be good for my sanity.

Some more details:
The issue is sometimes fixed when loading the video. It is so far always fixed if I load the video and then flick back and forth a few frames.

Note that in the screenshots above, the total number of frames does not correspond to the video length - seems like an issue with the order of the axes when the video is not loaded?

[niksirbi]

Alright, I've addressed all of @lochhh's comments (thanks for the review!) and documented the known segfault issue at the bottom of the guide.

Now it only remains to investigates @sfmig's issue.

[niksirbi]

Just noticed a potential issue and wanted to check if it is only on my side. I didn't realise before.

I can reproduce it, so it's not just your setup!

I haven't fully confirmed this but I have a suspicion:

As far as I can tell, this only happens with SLEAP multi-animal datasets. Interestingly, it happens with the "mixed-labels" version of the Aeon dataset, but not with the "proofread" one. I wonder whether this is somehow related to mixed identities/tracks in SLEAP files. Intriguingly, whether we are using the analysis.h5 or the .slp file doesn't matter. I will not get to the bottom of this tonight. Any ideas @lochhh?

[sfmig]

As far as I can tell, this only happens with SLEAP multi-animal datasets.

To me it happens with all the multi-animal datasets, not only SLEAP ones (the Sherlock bboxes dataset - which is not in GIN but can share- , and the DLC two mice dataset). I think maybe that is a stronger hint.

Another hint is that the frame slider end is set to 512 in all cases - seems like a default? It makes me think napari is interpreting the axes wrong... Just to be extra clear I am not loading any videos, just data.

[niksirbi]

To me it happens with all the multi-animal datasets, not only SLEAP ones (the Sherlock bboxes dataset - which is not in GIN but can share- , and the DLC two mice dataset). I think maybe that is a stronger hint.

Another hint is that the frame slider end is set to 512 in all cases - seems like a default? It makes me think napari is interpreting the axes wrong... Just to be extra clear I am not loading any videos, just data.

Yay, you make a strong case. If it's all multi-animal datasets, and the problem is solved by pre-loading a video first, it is indeed highly likely that napari does something funky with the axes there. I guess if the video is added first and the points later, napari aligns the axes of the points layer to the image layer? Do you know if pre-loading the image only (still frame) also solves this or do we always need the video?

[sfmig]

Do you know if pre-loading the image only (still frame) also solves this or do we always need the video?

I just tried with the two mice octagon data - the problem still exists if you load a still frame. The number of frames in the slider does not match the number of frames in the data.

[lochhh]

Wdyt about adding a link to issues tagged with the GUI and BUG labels here so that we can merge this docs PR and investigate the issues in another? @sfmig @niksirbi

[niksirbi]

Absolutely. This would also avoid the branching-on-branches mess we are in right now.

I propose the following course of action:

• merge this PR into napari-dev, with the added link to GUI-tagged issues (I will do this)
• rebase and merge Replace brainglobe-utils with qt-niu #441 into napari-dev (@lochhh will do this)
• Update napari-dev branch to make sure it's in-sync with main (I will do this)
• Deal with Unexpected "tail" from previous time frame showing in points layer in multi-animal datasets #446 in a separate-PR to napari-dev
• Once bug is fixed, merge napari-dev to main.

What do y'all think?

[sfmig]

Yeah excellent idea, thanks for suggesting @lochhh and for planning @niksirbi 🚀

I can continue to inspect this further and report any findings in issue #446

[lochhh]

Sounds good to me, although I'd prefer merging napari-dev to main sooner rather than waiting for all bugs to be fixed - we'll always discover more 🙈

[niksirbi]

We could also go for merging napari-dev to main before fixing the bug, which would be the bolder choice (meaning that we risk including a buggy plugin in the next release), but would make our dev processes easier.

[niksirbi]

I wrote my last message before reading your replies, sounds like we all agree, I will start the untangling now.

[niksirbi]

Bold it is!

[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