-
Notifications
You must be signed in to change notification settings - Fork 250
Miscellaneous fixes to SlideBook7Reader.java #4294
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
Conversation
added the code for getUsedFile added support for compress sldyz/npyz files
Fixed GetVoxelSize
thumb nails are unsigned integers, and exceeded 2^31, so I changed their size to Long
… 2^31 fixed more errors when trying to decode an integer value greater than 2^31
Added a comment
…taStore Added code to import the ROI annotations and store them in the MetadataStore. tested ok in fiji by me and out slidebook project manager
decode: yaml file decoding, added Long and LongVector types to read file addresses, pointers and pixmap values: changed Inter to Long
moved valueOf functions for Integer,Double,Float,inside try block to catch conversion errors.
compression: added support for slides stored with compression - zstd ,zlib,Lz4, RLE (Run length Encoded), added .sldyz and .npyz suffixes to recognize compressed slides.
fixed erroneous second argument for class String substring method.
fixed retrieval of voxel size
added support for slide files that have all 2D timepoint in the same npy file (SFMT - Single File Multiple Timepoints).
metadata: added support for ROI annotations
committed by mistake
Added a Long Type to fix errors in the metadata reader caused by reading integers greater than 2^31 Enclosed in try/catch all decoding of numbers from the yaml files Added support for sldyz and npyz files, which store the data compressed, using the zstd codecs Added support of 2D timelapse dats stored in a single file (SFMT -> Single File Multi Timpoint) Fixed GetVoxelSize to accont for magnification Fixed bugs in the usage of the string substring method Fixed allocation of byte buffer to read a plane. Allocation was twice as large as needed. Added code to import ROI's annotations when Metadata level is not Minimum Fixed adding Plane Exposure Time to metadata store Fixed error: "YAMLException: The incoming YAML document exceeds the limit: 3145728 code points." for very large yaml files by using yaml LoaderOptions setCodePointLimit.
|
Thanks for opening this, @nicolapapp, and apologies for the delay in response. As yesterday's Bio-Formats CI announcement [1] indicates, all pull request review is currently on hold due to ongoing infrastructure challenges. In order to review this pull request, we would need test data that fully covers the changes here. That would be particularly important for the new zstd support, but we generally expect test data to be provided which demonstrates why a particular fix was needed. Current guidelines on submitting test data are available at [2]. Given past discussions, an alternative would be to fully migrate SlideBook7Reader to an “external” reader. This is similar to what is done for SlideBook6Reader already. SlideBook7Reader is already labeled as an external reader, allowing it to be distributed separately from the rest of Bio-Formats [3]. To finish the work of making this an external reader would mean:
You would then be able to update and distribute the reader according to your own needs and timelines, without needing to go through our review process or provide test data. Since we would no longer be including SlideBook7Reader in core Bio-Formats, you would need to provide your own solutions for distributing updated jars that include this reader - presumably that is already being done for SlideBook6Reader though. Please let us know which approach you would prefer, or if you have any questions. As noted in the announcement above, we will come back to this once we have a solution to our current CI issues. [1] https://forum.image.sc/t/bio-formats-infrastructure-development-updates/110852 |
|
I have uploaded the test files to zenodo: |
|
Hi @melissalinkert I just wanted to check in on this pull request as our customers continue to ask us where it stands. We uploaded test data and it appears the checks have passed, so it would help to know if there is anything else you need from us. Thanks again! |
|
Thanks for the reminder, @bodenste. I simply had not had time to review this in any detail due to other priorities. In looking at the test datasets in https://zenodo.org/records/15235312, I can see that most of the provided datasets fail to open without this pull request. With this pull request, the files seem to open using However, I could not see a dataset which demonstrated the ROI/annotation changes. The OME-XML shown by Acknowledging that there are improvements here which don’t appear to break anything else, I am inclined to merge this. Going forward, though, the plan for this reader is:
This is really meant to help all of us - we’re not able to review on the timelines you need, and don’t feel that we have the knowledge or understanding needed to properly review and test proposed changes. Moving Please let us know whether you prefer option (a) or (b) above. It’s likely to be a few weeks before we proceed with migration or removal, though. |
|
Thanks for the good news @melissalinkert! We look forward to the changes being merged. That said, I'm unclear on the what the steps going forward mean. Our one and only goal is that "anyone out of the box can just run Fiji (and all other packages that use Bio-Formats) and open SlideBook data." What you describe sounds like the opposite of our goal, and is something that we want. I assume if things fork then support for this reader will simply vanish, unless they contact us for a library? I took a look at the list of "external" readers and it was not clear to me what differentiates "external" from "not external." Are you saying that no other corporate vendor assists with the maintenance of their file formats (other than the four listed as "external")? Again, apologies if I'm confused as I might well be, but I can say our users are in no way going to be happy about this. Now that we understand you need some better descriptions and some same data, I'm fairly certain this should be straightforward in the future. We certainly don't have have any special time constraints - these changes have been pending for multiple years (obviously due to confusion and lack of files). Anyway if you can better explain what you're proposing I'd appreciate it. We are looking for the most seamless way for the substantial group of "researchers who use SlideBook" and "researchers who use Bio-Formats to open their SlideBook data" have a straightforward experience. At the end of the we have hundreds and hundreds of mutual users, people who use this software every day. |
The current state is that we are not performing any maintenance work on the SlideBook 7 reader - all work on this reader is coming from your team. Our intention is to move this reader to some other repository that you can continue to work in. That can (and ideally should!) still be public. The only differences are that:
That means that you decide the level of support for the SlideBook 7 reader without any required input from OME.
An “external” reader is a Bio-Formats extension which is neither maintained nor distributed by the Bio-Formats team. Anything that is not external is included in the core Bio-Formats repository, and is subject to our review process and all of our contribution policies. The “external” reader feature was in fact added in #2496 to allow the SlideBook 6 reader to be developed separately (but still recognized by Bio-Formats). SlideBook 7 is indeed a bit of a special case - most other readers have no contribution whatsoever from the relevant vendor, much less primary maintainership.
We’ve definitely had the impression from comments here and private emails that there is an expected timeline (or at least a strong sense of urgency), which we are unable to meet. Perhaps that has been a misunderstanding, and if so our apologies for our part in this. However, that does not affect our policy or the path forward we are proposing.
Hopefully the above helps, but to summarize:
Fully understood, but please understand as well that our primary maintenance team consists of two people at the moment, who need to support the 100s of thousands of users of all Bio-Formats features. We provide support on a best effort basis, and are simply not able to do everything, so we’re trying to find a way to let you continue to make improvements to SlideBook support without our resource constraints holding you back. |
|
We have decided internally to delay the reader migration until a future release, to allow time for further discussion with the 3i team. See also documentation around this topic in https://bio-formats.readthedocs.io/en/latest/developers/external-readers.html. |
Added a Long Type to fix errors in the metadata reader caused by reading integers greater than 2^31
Enclosed in try/catch all decoding of numbers from the yaml files
Added support for sldyz and npyz files, which store the data compressed, using the zstd codecs
Added support of 2D timelapse data stored in a single file (SFMT -> Single File Multi Timpoint)
Fixed GetVoxelSize to accont for magnification
Fixed bugs in the usage of the string substring method
Fixed allocation of byte buffer to read a plane. Allocation was twice as large as needed.
Added code to import ROI's annotations when Metadata level is not Minimum
Fixed adding Plane Exposure Time to metadata store
Fixed error: "YAMLException: The incoming YAML document exceeds the limit: 3145728 code points." for very large yaml files by using yaml LoaderOptions setCodePointLimit.