-
Notifications
You must be signed in to change notification settings - Fork 10
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
Qt widget for loading pose datasets as napari Points layers #253
Conversation
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## napari-dev #253 +/- ##
==============================================
+ Coverage 99.78% 99.80% +0.02%
==============================================
Files 16 18 +2
Lines 931 1051 +120
==============================================
+ Hits 929 1049 +120
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
ae2b480
to
39d6e7e
Compare
800a157
to
6d6fe71
Compare
7aabffe
to
fae3f25
Compare
Regarding the un-tested |
* initialise napari plugin development * initialise napari plugin development * create skeleton for napari plugin with collapsible widgets * add basic widget smoke tests and allow headless testing * do not depend on napari from pip * include napari option in install instructions * make meta_widget module private * pin atlasapi version to avoid unnecessary dependencies * pin napari >= 0.4.19 from conda-forge * switched to pip install of napari[all] * seperation of concerns in widget tests * add pytest-mock dev dependency
Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk>
Hey @lochhh thank you for the comprehensive review! I've basically implemented all of your excellent suggestions, so have a look and see if you're happy with how I've patched things. I made a bit of a mess with syncing the 3 branches - Regarding your additional points:
Good point, it's distracting. I guess this is sneaking in through the
Does selecting
Excellent idea! I've opened this as a separate small PR. I don't really like the word "gui" because it's an acronym. How do we feel about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @niksirbi for addressing all comments/suggestions. Just a couple more suggestions and we're good to go 🚀
There is no option to load an image layer, except via "new image from clipboard". I can't drag and drop either.
Does selecting
FIle
>Open File(s)...
from the topnapari
menu work? For images that should work out of the box. For videos, it will prompt you to choose a plugin (i.e.napari-video
) if you have that installed. Let me know if this works and I'll make it part of the user guide.
This method works. When loading videos, there was no prompt to choose any plugins. I currently have 4 plugins installed: brainglobe-utils, movement, napari-console, and napari-svg. I also tried opening the single mouse EPM video - it crashed. I then tried with the smaller AEON 3 mice video and loaded the poses on top, this worked!
Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk>
Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk>
Quality Gate passedIssues Measures |
Thanks, again! Suggestions accepted as is.
Hmm I'll have to look more into the video thing, we already have an issue for that: #49 Anyway, my minimal goal with this PR was being able to overlay poses as I will squash-merge this into |
* initialise napari plugin development * Create skeleton for napari plugin with collapsible widgets (#218) * initialise napari plugin development * initialise napari plugin development * create skeleton for napari plugin with collapsible widgets * add basic widget smoke tests and allow headless testing * do not depend on napari from pip * include napari option in install instructions * make meta_widget module private * pin atlasapi version to avoid unnecessary dependencies * pin napari >= 0.4.19 from conda-forge * switched to pip install of napari[all] * seperation of concerns in widget tests * add pytest-mock dev dependency * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * Added loader widget for poses * update widget tests * simplify dependency on brainglobe-utils * consistent monospace formatting for movement in public docstrings * get rid of code that's only relevant for displaying Tracks * enable visibility of napari layer tooltips * renamed widget to PosesLoader * make cmap optional in set_color_by method * wrote unit tests for napari convert module * wrote unit-tests for the layer styles module * linkcheck ignore zenodo redirects * move _sample_colormap out of PointsStyle class * small refactoring in the loader widget * Expand tests for loader widget * added comments and docstrings to napari plugin tests * refactored all napari tests into separate unit test folder * added napari-video to dependencies * replaced deprecated edge_width with border_width * got rid of widget pytest fixtures * remove duplicate word from docstring * remove napari-video dependency * include napari extras in docs requirements * add test for _on_browse_clicked method * getOpenFileName returns tuple, not str * simplify poses_to_napari_tracks Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * [pre-commit.ci] pre-commit autoupdate (#338) updates: - [github.com/astral-sh/ruff-pre-commit: v0.6.9 → v0.7.2](astral-sh/ruff-pre-commit@v0.6.9...v0.7.2) - [github.com/pre-commit/mirrors-mypy: v1.11.2 → v1.13.0](pre-commit/mirrors-mypy@v1.11.2...v1.13.0) - [github.com/mgedmin/check-manifest: 0.49 → 0.50](mgedmin/check-manifest@0.49...0.50) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Implement `compute_speed` and `compute_path_length` (#280) * implement compute_speed and compute_path_length functions * added speed to existing kinematics unit test * rewrote compute_path_length with various nan policies * unit test compute_path_length across time ranges * fixed and refactor compute_path_length and its tests * fixed docstring for compute_path_length * Accept suggestion on docstring wording Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * Remove print statement from test Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * Ensure nan report is printed Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * adapt warning message match in test * change 'any' to 'all' * uniform wording across path length docstrings * (mostly) leave time range validation to xarray slice * refactored parameters for test across time ranges * simplified test for path lenght with nans * replace drop policy with ffill * remove B905 ruff rule * make pre-commit happy --------- Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * avoid redefining duplicate attributes in child dataclass * modify test case to match poses_to_napari_tracks simplification * expected_log_messages should be a subset of captured messages Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * fix typo Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * use names for Qwidgets * reorganised test_valid_poses_to_napari_tracks * parametrised layer style tests * delet integration test which was reintroduced after conflict resolution * added test about file filters * deleted obsolete loader widget file (had snuck back in due to conflict merging) * combine tests for button callouts Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * Simplify test_layer_style_as_kwargs Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> --------- Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* initialise napari plugin development * Create skeleton for napari plugin with collapsible widgets (#218) * initialise napari plugin development * initialise napari plugin development * create skeleton for napari plugin with collapsible widgets * add basic widget smoke tests and allow headless testing * do not depend on napari from pip * include napari option in install instructions * make meta_widget module private * pin atlasapi version to avoid unnecessary dependencies * pin napari >= 0.4.19 from conda-forge * switched to pip install of napari[all] * seperation of concerns in widget tests * add pytest-mock dev dependency * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * Added loader widget for poses * update widget tests * simplify dependency on brainglobe-utils * consistent monospace formatting for movement in public docstrings * get rid of code that's only relevant for displaying Tracks * enable visibility of napari layer tooltips * renamed widget to PosesLoader * make cmap optional in set_color_by method * wrote unit tests for napari convert module * wrote unit-tests for the layer styles module * linkcheck ignore zenodo redirects * move _sample_colormap out of PointsStyle class * small refactoring in the loader widget * Expand tests for loader widget * added comments and docstrings to napari plugin tests * refactored all napari tests into separate unit test folder * added napari-video to dependencies * replaced deprecated edge_width with border_width * got rid of widget pytest fixtures * remove duplicate word from docstring * remove napari-video dependency * include napari extras in docs requirements * add test for _on_browse_clicked method * getOpenFileName returns tuple, not str * simplify poses_to_napari_tracks Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * [pre-commit.ci] pre-commit autoupdate (#338) updates: - [github.com/astral-sh/ruff-pre-commit: v0.6.9 → v0.7.2](astral-sh/ruff-pre-commit@v0.6.9...v0.7.2) - [github.com/pre-commit/mirrors-mypy: v1.11.2 → v1.13.0](pre-commit/mirrors-mypy@v1.11.2...v1.13.0) - [github.com/mgedmin/check-manifest: 0.49 → 0.50](mgedmin/check-manifest@0.49...0.50) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Implement `compute_speed` and `compute_path_length` (#280) * implement compute_speed and compute_path_length functions * added speed to existing kinematics unit test * rewrote compute_path_length with various nan policies * unit test compute_path_length across time ranges * fixed and refactor compute_path_length and its tests * fixed docstring for compute_path_length * Accept suggestion on docstring wording Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * Remove print statement from test Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * Ensure nan report is printed Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * adapt warning message match in test * change 'any' to 'all' * uniform wording across path length docstrings * (mostly) leave time range validation to xarray slice * refactored parameters for test across time ranges * simplified test for path lenght with nans * replace drop policy with ffill * remove B905 ruff rule * make pre-commit happy --------- Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * initialise napari plugin development * avoid redefining duplicate attributes in child dataclass * modify test case to match poses_to_napari_tracks simplification * expected_log_messages should be a subset of captured messages Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * fix typo Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * use names for Qwidgets * reorganised test_valid_poses_to_napari_tracks * parametrised layer style tests * delet integration test which was reintroduced after conflict resolution * added test about file filters * deleted obsolete loader widget file (had snuck back in due to conflict merging) * combine tests for button callouts Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> * Simplify test_layer_style_as_kwargs Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> --------- Co-authored-by: Chang Huan Lo <changhuan.lo@ucl.ac.uk> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
What is this PR
Why is this PR needed?
This is the 2nd in a series of multiple PRs (following #218), in which I'll try breaking down the work-in-progress contained in #112, and debug any obstacles I encounter along the way. These PRs will be merged into the
napari-dev
branch, until the plugin becomes minimally functional for users, at which point we'll merge thenapari-dev
branch intomain
.This PR closes #47.
What does this PR do?
It replaces the placeholder "hello" Qt widget with an actual widget that can load
movement
poses dataset into napari as a Points layer, via functions defined innapari/convert.py
. Theposition
data variable is represented in the Points array, whileconfidence
is stored in the layer properties, alongside individual and keypoint names. This means that when hovering over any point in napari, one can see these properties displayed.In future PRs, the properties will be used to control the appearance of points (i.e. mapping various properties to point size, colour, etc.) For now, the points have a fixed style (coloured by individual ID for multi-individual datasets, by keypoint ID for single-individual datasets). To facilitate the consistent styling of these points, I've defined dataclasses in
napari/layer_styles.py
, where we can store the default styling per layer type.Currently, the plugin with the poses loader widget looks like this:
What does this PR NOT do?
These features, though planned, are not essential enough to be part of the "napari prototype" #31:
Code structure
The new modules all reside within the
movement/napari
folder, and include:_meta_widget.py
: this creates a container of collapsible widgets (imported frombrainglobe-utils
). Within this container we can stack many widgets, each handling a different task/workflow step. All widgets except for the currently expanded (active) one will appear collapsed. For now this container only houses one widget - the poses loader (see next point)._loader_widgets.py
: contains thePosesLoader
class, which defines the first (and for now, only) collapsible widget.PosesLoader
is essentially a frontend for theload_poses.from_file()
function. When a file is being loaded, the_on_load_clicked
method will call theposes_to_napari_tracks
function (see next point) which does all the data wrangling required to transform amovement
poses dataset into napari compatible array + properties.convert.py
: which contains the aforementionedposes_to_napari_tracks
function. The idea is that this module will house all movenet->napari and napari->movement conversion utilities. There's something I need to clarify here: why am I creating the data for anapari
Tracks layer if my intention is to create a Points layer? That's because the data structure for both of these the same: Points are specified as an array with[z, y, x]
columns (in out case that's[time, y, x]
), whereas Tracks require columns[track_id, time, y, x]
. So by creating a Tracks array, we maintain the track_id information (which we will need in the future anyway), and we get a Points array for "free" (by taking the last 3 columns).layer_styles.py
: dataclasses defining styles for napari layers. Here I've created a baseLayerStyle
class with attributes that are common across layer types, and aPointsStyle
child class with attributes specific to Points layers. The latter class implements aset_color_by
method, which simplifies the re-coloring of the points according to a chosen property (e.g. keypoint or individual).New unit test modules exactly mirror the above files (see below).
References
After the current PR is merged, the only absolutely required issue for completing the prototype would be #283. After we have a guide to using the napari plugin, we can merge the prototype to
main
, and releasev0.1
of movement.How has this PR been tested?
Unit tests have been written for all new modules, and can be found in the
tests/test_unit/test_napari_plugin
folder.The test modules map 1-to-1 to the aforementioned new code modules, so:
test_meta_widget.py
test_poses_loader_widget.py
test_convert.py
test_layers_styles.py
There is one untested widget method,PosesLoader._on_browse_clicked
which opens a file dialog to select a poses file. Despite spending lots of time on this, I couldn't figure out a way to successfully mock all the actions required for testing the file dialog, without actually opening one. Suggestions welcome. Alternatively, this can be opened as a separate issue. I basically decided to open this PR for review without this unit test, because the whole matter was dragging for far too long, and perhaps testing that widget is not so important.How to review this PR
movement
from this branch withdev
dependencies:pip install -e .[dev]
. This will also include the optional dependencies specified under thenapari
extra.napari
with themovement
plugin docked:napari -w movement
.~/.movement/data
).~/.movement/data/frames
. This should always work, give you are using the right image for the poses filenapari-video
plugin, and drag and drop one of the videos found in~/.movement/data/videos
(choose to open it with the video plugin when napari prompts you). Initially I'd planned to includenapari-video
as a dependency, but I ran into issues during CI. We have to find a way around these as part of Napari plugin reader for videos #49. For what it's worth, the video plugin currently works fine in my M2 Macbook, is laggy on Ubuntu, and fails on Ubuntu CI. See below in "Known issues" for more info.Known issues
napari-video
plugin currently doesn't perform reliably cross-platform, see related issues:opencv-python-headless
also for newer versions of Python? postpop/videoreader#6Codecov complains because of the aforementioned lack of tests forPosesLoader._on_browse_clicked
Because reviewing this PR will take a long time, I suggest @lochhh and @sfmig you start to slowly work your way through it, while I focus on solving these known issue in parallel.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
Yes, but this will be done as part of #283.
Checklist: