-
Notifications
You must be signed in to change notification settings - Fork 17
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
Update precompressed tile documentation #390
Conversation
Add note on lossy or lossless compression to precompressed.rst
…o precompressed-page
…rmats-documentation into precompressed-page
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.
That's a great starting point for documenting the pre-compressed tile reading/writing feature especially if this is deemed to be a major feature of the upcoming release.
A few inline suggestions and a wider discussion about where to keep track of the list of supported reader/writers.
* tile sizes cannot be changed during conversion | ||
* compression type cannot be changed during conversion | ||
* input and output format must support same compression type and tile size | ||
* not all formats supported by Bio-Formats currently allow reading/writing precompressed tiles |
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.
At first read, this felt slightly redundant with the previous bullet point even though the message is different.
Also maybe this needs to be stated as in. the input format must have precompressed tiles reading support and the output format must have precompressed tiles writing support
with a reference to the paragraph about the supported formats
* not all formats supported by Bio-Formats currently allow reading/writing precompressed tiles | ||
* tools other than :program:`bfconvert` do not currently make use of the precompressed tile API | ||
|
||
Formats that support precompressed tile reading: |
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.
Assuming additional formats get added to this list mid-term, https://bio-formats.readthedocs.io/en/latest/formats/index.html would probably the natural section to keep a reference of all readers/writers supporting precompressed tile reading/writing. If a developer page gets introduced about precompressed tile reading/writing, this would also be more natural as a link rather than adding a reference to a subsection of a command-line utility page.
Reviewing the few available options:
- https://bio-formats.readthedocs.io/en/latest/supported-formats.html includes an export column which describes the writing status (although this is definitely not obvious). Adding 2 more columns to this already heavy table would probably be detrimental to the overall readability
- https://bio-formats.readthedocs.io/en/latest/formats/options.html describes additional reader/writing options. This was introduced with regards to
DynamicMetadataOptions
but could include additional reader/writer specificity - a new page listing reader/writers with pre-compressed tile support
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 tried to split into two pages - one under /formats/
that has the high-level summary and list of supported readers/writers, and the original one under /users/comlinetools/
that's more bfconvert-specific. Open to other thoughts though if that doesn't make sense.
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.
Sorry for the delay here. Coming back to this as we are heading towards Bio-Formats 8.0.0. Two inline comments to get this production ready and then proposing to include
sphinx/formats/precompressed.rst
Outdated
----------------------------------------------- | ||
|
||
* SVS (since 7.1.0) | ||
* NDPI (forthcoming: https://github.com/ome/bioformats/pull/4181) |
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.
Given the current roadmap, I would change this and all the below to since 8.0.0
(or use Sphinx versionadded
markup but it might not be the best for this page)
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.
Done in 505cc2f.
"Precompressed" tiles | ||
===================== | ||
|
||
See first :doc:`a summary of the precompressed tiles feature </formats/precompressed>`. |
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.
Interestingly the conversion.rst
page also mentions a Summary of the precompressed tiles feature
but links to this page. Should this be a summary of the formats supporting precompressed tiles features
instead?
Or alternatively is this not necessary as the link below links directly to the readers/writers section?
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.
My original thought here was that formats/precompressed.rst
is the higher-level motivation for why you might want to use precompressed tiles, where users/comlinetools/precompressed.rst
and users/comlinetools/conversion.rst
are talking about the specific implementation in bfconvert
. So if you didn't already know why this feature might be useful, you'd want to read at least the first section of formats/precompressed.rst
to understand.
4d06960 tries to clarify the link text a bit, but open to other suggestions.
See ome/bioformats#4190 (review). This is mostly for users of bfconvert at the moment; a next step (here or in a new PR) would be something more useful to developers.