-
Notifications
You must be signed in to change notification settings - Fork 13
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
Clean up tests and reorganize test artifacts #58
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.
I liked the README in the artifacts directory and the layout of conftest.py. Looks good to me!
Co-authored-by: Evan Kiefl <ekiefl@users.noreply.github.com> Signed-off-by: Keith Cheveralls <keith.chev@gmail.com>
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.
Tests look great! I left a few questions/comments for the artifacts/README.
The changes to the tests are so substantial that there is no diff; the tests appear as new files.
No diff bc the name changed from tests/test_readlif.py
--> tests/test_reader.py
.
tests/artifacts/README.MD
Outdated
|
||
### `xyzt-example/xyzt-example.lif` | ||
|
||
Contains a single image. 1024 x 1024. 8-bit. Shape: C2, T4, Z3. |
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 it's T3.
Side note: the channel dimension seems to be missing from Dims
?
>>> lif_file.image_list
[{'dims': Dims(x=1024, y=1024, z=3, t=3, m=1),
'display_dims': (1, 2),
'dims_n': {1: 1024, 2: 1024, 3: 3, 4: 3},
'scale_n': {1: 9.8709062997224,
2: 9.8709062997224,
3: 2.9789060682401844,
4: 0.19273393080851883},
'path': 'xyzt test/',
'name': 'Series002',
'channels': 2,
'scale': (9.8709062997224,
9.8709062997224,
2.9789060682401844,
0.19273393080851883),
'bit_depth': (8, 8),
'mosaic_position': [],
'channel_as_second_dim': False,
'settings': {}}]
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.
Yeah, the dims
attribute contains all of the dimensions other than the channel dimension, which appears in the separate channels
attribute. I think this is because the channels are stored separately in the LIF metadata, but I'm not sure.
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.
Also, dims
only contains XYZTM
, so it doesn't include the lambda-scan-related dimensions (excitation and emission wavelengths).
Context
Before addressing the open PRs, I wanted to improve the test coverage. Before doing that, I had to understand the existing tests, and to do that, I felt it made sense to rewrite the tests for clarity, since they were a bit hard to follow.
Overview
This PR cleans up the tests by rewriting them to improve nomenclature and clarity. It also slightly expands the tests where it was possible to include some trivial additional cases/tests. Some tests were also split into two tests, in cases where different functionality was being tested in the same test.
It also reorganizes the test artifacts to clarify the relationship between some of the LIF files and their expected outputs (which are stored in the form of TIFF files).
Finally, it adds a README to
/tests/artifacts
with a brief explanation of what each test LIF file contains (as determined by manually inspecting how it is loaded by BioFormats in Fiji).Note for reviewers
The changes to the tests are so substantial that there is no diff; the tests appear as new files. FWIW, I was careful not to change the substance of the tests or to remove any tests.