-
Notifications
You must be signed in to change notification settings - Fork 1
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
Tests for the image_io sub-module #48
Merged
+282
−51
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
75908fe
add docstrings for tests and update to use tmp_path
K-Meech fd64f9c
update docstrings and add nrrd test
K-Meech dbd9962
test load_any and 3D tiff
K-Meech 407abc5
merge upstream changes
K-Meech 3c4f1e5
test 3D tiff scaling
K-Meech b064e33
test txt sequence sorting and nii to numpy
K-Meech 61c7caa
test tiff sequence scaling and parallel loading
K-Meech 799f52e
add tests for image size
K-Meech 12da288
tidy up tests
K-Meech b3bd485
split shuffle into a fixture
K-Meech File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev
Previous commit
Loading status checks…
split shuffle into a fixture
- Loading branch information
commit b3bd48578d848b9215b747ea1c7105006364c28e
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Is it worth to briefly mention a typical use case for that text file?
Reading this, I understood what was going on, but wasn't sure why.
[Edit:] after reviewing the rest of the PR, my understanding as to why we need this is that we support this input format, but never write to it, so we want to have this to test. Presumably this format is useful for parallel/lazy IO? Could this be a fixture just for consistency - so all our test code is fixtures or test functions?
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.
Yes that's right - it's needed as we support reading from a txt file with
load_img_sequence
, but there's no corresponding save function. I'm not sure what the use-case is for such a text file - I guess if you have tiff files spread over multiple directories it could be useful?I'll look at changing this into a fixture 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.
@alessandrofelder - I'm trying to convert this to a fixture, but having problems with allowing it to be optionally shuffled like here. Is there a simple way to pass a parameter like
shuffle=True
to a fixture? Otherwise, I could have two fixtures - one for a shuffled text file and one for not, but they will have pretty much identical code in both unless I refactor it out into some kind of helper function. Any ideas?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.
Fixtures can't take optional arguments, but can be indirectly parameterised:
will pass, for example. However if you change
reps: int
toreps: int = 2
in the fixture definition, the invoking pytest will fail.However I don't think
write_tiff_sequence_with_txt_file
can be made into a fixture (depending on how you're using it). I'm prepared to be proven wrong though 😅 But in my understanding, fixtures have to either:Here,
write_tiff_sequence_with_txt_file
needs to be run in the middle of a test, so it has to be a function, and can't be a fixture. You could try redesigning it as a setup fixture though - assuming you're only ever using it at the start of a test, before anything else runs:Otherwise, I don't think it can be "fixture-ised".
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.
Thanks for the info @willGraham01! That makes it a lot clearer.
I think I could redesign it as a setup fixture, but this would mean I'd need a separate test case for txt files with
load_any
, rather than having it as a parameter hereAs we support loading txt files in
image_io
viaload_any
andload_img_sequence
, perhaps it would make sense for this to be moved intoimage_io
to allow saving this kind of file also? Then the test functions could simply call this?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.
Interesting. Yea fixture parameters are tricky!
In this specific case, IIUC we need the
shuffle=True
part for testing thesort
argument ofload_img_sequence
only, whereas we will need theshuffle=False
case in the source code (as part of addressing #50). I don't think we'd ever want to intentionally shuffle the order of paths in a text file in our source code?So I'd suggest
write_tiff_sequence_with_txt_file
not a fixtureshuffle
part into a fixture that overwrites the textfile with a shuffled version of it.write_tiff_sequence_with_txt_file
to the source codeIs that sensible?
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.
Sounds good to me! I'll add these points to the issue + split the
shuffle
part nowThere 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.
@alessandrofelder I split
shuffle
into a fixture now. Could you take a look + merge if you're happy with it? Then I'll work on #50