niksirbi commented 2 weeks ago

Description

What is this PR

[ ] Bug fix
[x] Addition of a new feature
[ ] Other

Why is this PR needed?

As explained in #31, I've shifted strategy for developing the napari plugin. This is the first in a series of multiple PRs, in which I'll try breaking down the work-in-progress contained in #112, and debug any obstacles I encounter along the way. The PRs will be merged into the napari-dev branch, until the plugin becomes minimally functional for users, at which point we'll merge the napari-dev branch into main.

What does this PR do?

It sets up all the necessary components for a napari plugin, including optional dependencies, the napari.yaml file, a simple collapsible widget (which for now just includes a simple "hello, world" button as a placeholder), and some rudimentary smoke tests.

I struggled with finding the right way to install napari, such that it works across OSes (both locally and in CI). While debugging this, I stumbled on an issue with the brainglobe-utils.

In the end, pip install napari[all] seems to have worked across the platforms we test. For some reason, it doesn't work on recent Ubuntu versions (like my local Ubuntu 24.04), but I think I'm going to ignore this issue for now, open an issue about it, and wait for napari to deal with it.

The optional dependencies included in movement[napari] are :

napari = [
  "napari[all]>=0.4.19",
  # the rest will be replaced by brainglobe-utils[qt]>=0.6 after release
  "brainglobe-atlasapi>=2.0.7",
  "brainglobe-utils>=0.5",
  "qtpy",
  "superqt",
]

but, after https://github.com/brainglobe/brainglobe-utils/pull/84 is merged and a new version of brainglobe-utils is released, that can change to just napari = ["napari[all]>=0.4.19", brainglobe-utils[qt]>=0.6"].

The installation instruction and tox configuration have been updated accordingly (see sections on testing and docs below).

References

31

112

https://github.com/brainglobe/brainglobe-utils/pull/84

How has this PR been tested?

Tests have been added for the napari widget. The tox config has been modified to pass the right env variables for QT tests to work headlessly in CI. Additionally, an extra action has been added to the test workflow to facilitate this.

Is this a breaking change?

No.

Does this PR require an update to the documentation?

Yes, and the installation instructions have been updated both in the README and the more detailed installation guide in the docs.

Checklist:

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

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.71%. Comparing base (7bb1ecf) to head (78983f6).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## napari-dev #218 +/- ## ============================================== + Coverage 99.70% 99.71% +0.01% ============================================== Files 12 14 +2 Lines 678 702 +24 ============================================== + Hits 676 700 +24 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

niksirbi commented 2 weeks ago

tests fail on windows-latest runners with:

FAILED tests/test_unit/test_filtering.py::test_savgol_filter_with_nans - numpy.linalg.LinAlgError: SVD did not converge in Linear Least Squares

That's perplexing because this doesn't occur on any other OS, and also not on Windows on our other branches. My only guess is that the new dependencies cause us to end up with different versions of numpy (or with its conda-forge version instead of the pip version), and this old issue may have reared its head (though that should have been patched in 2021).

Maybe pinning numpy>1.20 will help?

niksirbi commented 1 week ago