-
Notifications
You must be signed in to change notification settings - Fork 0
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 file type: .topostats #53
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.
Thanks for the comprehensive work on this @SylviaWhittle great to see all the tests and in-line examples as well as the notebook.
Some minor comments, not all for addressing in this Pull Request, have tried to make suggestions where things could be changed though.
examples/example_01.ipynb
Outdated
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.
Not a big issue and certainly not something to do here but we could move the comments in the cells out to Markdown cells.
AFMReader/topostats.py
Outdated
data = unpack_hdf5(open_hdf5_file=f, group_path="/") | ||
|
||
file_version = data["topostats_file_version"] | ||
logger.info(f"TopoStats file version: {file_version}") |
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.
Perhaps include filename
as is done in the logger.error()
below so people know what file is which version?
logger.info(f"TopoStats file version: {file_version}") | |
logger.info(f"[{filename}] TopoStats file version : {file_version}") |
[ | ||
pytest.param( | ||
"sample_0.topostats", | ||
0.1, |
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.
Not for this PR but as we move forward with developing and standardising the .topostats
file format and defining versions we should be looking to document precisely what each file version corresponds to with a detailed technical specification. A useful article on this is A practical guide to writing technical specs - Stack Overflow
tests/test_topostats.py
Outdated
("file_name", "topostats_file_version", "image_shape", "pixel_to_nm_scaling", "data_keys", "image_sum"), | ||
[ | ||
pytest.param( | ||
"sample_0.topostats", |
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.
Perhaps pedantic but to make it easier to read/understand what's what in the tests/resources/
directory we could use a nomenclature that reflects the version.
"sample_0.topostats", | |
"sample_0_1.topostats", |
...would indicate the version 0.1
(would also require renaming sample_0.topostats
> sample_0_1.topostats
).
Thanks for testing this @MaxGamill-Sheffield
You commit 0fdb9cc introduced a couple of unintended changes to the notebook, the cell pre-commit install ...should do the trick. |
I think I've addressed the comments? Let me know if I've forgotten something. I think the documentation on the format could be done in another PR? Or should I add it to this one? |
Definitely a separate issue and one that would benefit from wider discussion as to what people actually want to have in |
Closes #51
This PR migrates support for the
.topostats
file format from TopoStats as part of our initiative to migrate all of TopoStats'io.py
over. (#22 )A function for reading hdf5 files and dumping the data into a dictionary is the main logic behind loading these files. Several tests have been added for it.