Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Testing is good #19
Testing is good #19
Changes from 9 commits
d4abf56
0c62a0e
08f825f
a8c6d12
15903d6
17da624
47455e4
d4f7fc8
8d23edd
572ea2e
6092b75
afabd1c
51de7ab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
L1 and L4 can be deleted as these imports are not being used
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.
I think pytest might be as I use a fixture from it, and bids will be used to validate that the outputs look correct.
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.
pytest
has temp dir fixtures (https://docs.pytest.org/en/6.2.x/tmpdir.html); not sure if that would be more advantageous to use than thetempfile
based implementationThere 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.
Yeah, pytest has everything, but despite my owning a book on it I'm not wholly familiar. I think we could move to using exclusively pytest, but for the moment it works and I'm able to easily inspect the temp dir if something fails.
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.
Okay, this is getting out of hand. I've gone ahead and created a petutils library that includes the sort of directory tree's we're intent on testing on. It has it's own fixtures that use pytest's tmpdir. Going to just merge this and switch over directory creation to the fixtures contained in in openneuropet/petutils for further development.
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.
Should we consider downsampling the test data to speed up the tests?
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.
From my understanding it's a registration issue with the current test data, either way I think we should optimize later and push through with it 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.
It would be good to check if the defaced PET file(s) is/are created.
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.
See my comment about the mask files, we're going to need to slightly modify the pipeline to output nifti masks instead of
mgz
, but that's irrelevant for the purposes of this branch/PR. I'd like to add all the tests even if it means everything is failing.