make check_training_data_exists more strict, and test it #528
Conversation
IgorTatarnikov
left a comment
IgorTatarnikov
left a comment
There was a problem hiding this comment.
Looks good! I found the toast at the bottom of the screen hard to catch, since this interrupts the workflow should it use display_info from qt-niu to show a pop up? I don't think it's required so I'll approve and let you decide!
|
As discussed internally, we should loosen the strictness on the layer emptiness as per @adamltyson's suggestion, to allow users to iteratively add points to a single training data layer, but probably warn users that their final retraining data should be a balanced mix of cells/non-cells |
alessandrofelder
force-pushed
the
improve-save-curation
branch
from
1d17a02 to
84b7473
Compare
IgorTatarnikov
left a comment
IgorTatarnikov
left a comment
There was a problem hiding this comment.
LGTM!
Just a few small comments.
| def test_valid_widget_has_valid_training_data(valid_curation_widget): | ||
| assert valid_curation_widget.check_training_data_exists() |
There was a problem hiding this comment.
Is this a duplicate of test_valid_widget_has_extractable_data?
There was a problem hiding this comment.
is_data_extractable is a slightly different function to check_training_data_exists so I think not (we should maybe refactor, but that should be a separate issue/PR?
tests/napari/test_curation.py
Outdated
| valid_curation_widget.viewer.layers["Training data (non cells)"].data = ( | ||
| None | ||
| ) | ||
| valid_curation_widget.check_training_data_exists() |
There was a problem hiding this comment.
Should we assert not valid_curation_widget.check_training_data_exists() just to make sure False is also returned?
There was a problem hiding this comment.
Yes (but it it should be assert not assert not - this case will still return True just also call show_info?)
|
Test failures are unrelated and fixed by #577 |
What is this PR
What does this PR do?
Makescheck_training_data_existsmore strict, by requiring both non-empty training data layers to return True.For
check_training_data_exists, this PRReferences
Closes #527
How has this PR been tested?
Local
pytestIs this a breaking change?
Yes - users can't get away with a non-existent training data layer anymore.
Does this PR require an update to the documentation?
Our tutorial still works the same, so no change needed there. This change enforces good practice with users.
Have added a docstring to a function that didn't have one.
Checklist: