-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add curation keyboard shortcuts #366
Conversation
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.
Looks good!
Just making a note here for our future selves and other interested people, that this PR mainly
- adds keyboard shortcuts
but also - replaces local functions with
brainglobe-utils
equivalents - removes tests for those functions
- makes tiny improvements to explicitness of code
- adds a "overwrite existing data?" warning.
cellfinder/napari/curation.py
Outdated
|
||
self.__delete_existing_saved_training_data() | ||
|
||
def __delete_existing_saved_training_data(self): |
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.
For my understanding, why do functions in the file start with two underscores?? Worth an issue if it's not intended.
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.
Double underscores are as close to private as you can get in Python. Not sure why they're used here and not anywhere else in BrainGlobe though.
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.
Ah cool! Didn't know. From reading that it sounds like the use case is mainly to make it impossible to override this in subclasses. We currently don't have any subclasses of CurationWidget
, and for consistency it would be good to not use it here, but then again not sure it's enough of a priority right now 🤔
TLDR I suggest leaving for now?
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.
Fine by me.
The tests are failing now, but is this due to #368? |
Yup, presumably. Just to be sure, shall we wait with merging until we resolve that? |
Yeah there's no rush for this |
Requires #365 to be merged first
Closes #364