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

Migrate cellfinder into this package #29

Merged
merged 355 commits into from
Dec 13, 2023
Merged

Migrate cellfinder into this package #29

merged 355 commits into from
Dec 13, 2023

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Nov 15, 2023

See brainglobe/BrainGlobe#45

Moves the existing cellfinder workflow into this package. Also then restructures the repository so that we can package the workflows without packaging the benchmarks and test data, to avoid users obtaining a bloated install.

To the reviewer:

Key info to check is in the README and pyproject. The remaining 6000-lines or so are just the new inclusion of what is currently in the cellfinder package.

On Merge:

  • Tag brainglobe-workflows version 1
  • Push to PyPI again

TODO List:

  • Update the missing links once the website is ready
  • Choose new name for cellfinder CLI tool and give it a temporary cellfinder alias
  • Ensure that ASV benchmarks are still working properly (didn't break any of @sfmig's stuff 😌 )
  • Ensure CI is passing (except CodeCov, but that's more an existing cellfinder problem)
  • Move Docker API secrets into this repo to allow for deployment (if we still want this?) Purge Docker images
  • Update README content and move old cellfinder content to somewhere appropriate on the website.

This reconciles the two divergent histories of the old `brainglobe-scripts` and `cellfinder` (CLI tool).
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 222 lines in your changes are missing coverage. Please review.

Comparison is base (e69e83e) 92.80% compared to head (ef35de9) 80.78%.

Files Patch % Lines
brainglobe_workflows/analyse/analyse.py 48.76% 62 Missing ⚠️
brainglobe_workflows/main.py 67.67% 32 Missing ⚠️
brainglobe_workflows/figures/heatmap.py 34.28% 23 Missing ⚠️
tests/cellfinder/conftest.py 38.23% 21 Missing ⚠️
tests/cellfinder/test_unit/test_tools/test_prep.py 52.63% 18 Missing ⚠️
brainglobe_workflows/extract/extract_cubes.py 91.62% 17 Missing ⚠️
brainglobe_workflows/tools/prep.py 90.53% 16 Missing ⚠️
...s/cellfinder/test_integration/test_registration.py 68.57% 11 Missing ⚠️
brainglobe_workflows/tools/image_processing.py 84.09% 7 Missing ⚠️
brainglobe_workflows/export/abc4d.py 33.33% 6 Missing ⚠️
... and 5 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #29       +/-   ##
===========================================
- Coverage   92.80%   80.78%   -12.02%     
===========================================
  Files           2       28       +26     
  Lines         125     1494     +1369     
===========================================
+ Hits          116     1207     +1091     
- Misses          9      287      +278     

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

@willGraham01 willGraham01 marked this pull request as ready for review November 22, 2023 10:58
Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Looks good - think I need some minor clarifications to help me understand things better!

.github/workflows/test_and_deploy.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
MANIFEST.in Outdated Show resolved Hide resolved
pyproject.toml Outdated
]
napari = ["napari[pyside2]", "brainglobe-napari-io", "cellfinder-napari<1.0.0"]
Copy link
Member

Choose a reason for hiding this comment

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

Probably also slightly out-of-scope for this PR but why do we ship a Qt backend for napari here, and the non-default one at that?

Copy link
Collaborator Author

@willGraham01 willGraham01 Nov 27, 2023

Choose a reason for hiding this comment

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

I'm not entirely sure - this seems to have been an artefact of cellfinder since before we moved to pyproject.toml too: https://github.com/brainglobe/cellfinder/pull/247/files#

Looking at brainreg, we're shipping napari with a backend (albeit Qt5) there too. As an experiment I'll change this to ship with Qt5 for consistency, but my answer to "why ship a backend" is an unsatisfactory appeal to tradition 😅

Copy link
Member

Choose a reason for hiding this comment

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

Cool - Opened brainglobe/BrainGlobe#53 for this!

Copy link
Member

Choose a reason for hiding this comment

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

I think we went with pyside2 due to some licensing issues, but TBH I think we're just ignoring that for now.

Don't we need to ship some back-end, otherwise there's an extra installation step for users?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't we need to ship some back-end, otherwise there's an extra installation step for users?

Yeah if we don't ship a backend it'll be an extra step.

Although this might mean that we're inadvertently overwriting their backend choice, if they have one installed already. EG If they have pyside installed and we then force-install napari[pyqt5], I'm not sure what that'll do to their env/machine

@willGraham01
Copy link
Collaborator Author

willGraham01 commented Nov 27, 2023

@alessandrofelder Have addressed the things you raised - thanks for catching them!

For the napari backend, I've set to using pyqt to be consistent with brainreg for the time being, until brainglobe/BrainGlobe#53 is taken further.

Tests seem to be passing but will take an hour to complete all done

Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Tiny suggestion, otherwise 👍

pyproject.toml Outdated Show resolved Hide resolved
Co-authored-by: Alessandro Felder <[email protected]>
@willGraham01 willGraham01 merged commit afba965 into main Dec 13, 2023
8 of 10 checks passed
@willGraham01 willGraham01 deleted the dev branch December 14, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants