Fix issues #100
Fix issues #100
Conversation
dominikl
force-pushed
the
fix_issues
branch
2 times, most recently
from
6ac291f to
13819a2
Compare
dominikl
force-pushed
the
fix_issues
branch
2 times, most recently
from
b4e7a7b to
680d7ac
Compare
|
Sorry for the PR/branch history, totally messed it up :-) And sorry @jo-mueller I didn't manage to keep the authorship of your commit. I've cherry-picked it and split it into two, one commit to add the test (680d7ac) and one commit to add the fix (f0e4caf), so you can see in the build history that it really fixes the issue; but lost you as original author. |
|
Hi @dominikl , thanks for working on this. I guess it can't be helped - at the end of the day, I'd rather have this working again than insisting on having my name inscribed on the frontpage of this repo ;) |
|
@dominikl Just wanted to ping this here - is there anything I could do that would help this going forward? |
|
Sorry, this PR was probably just missed because of Christmas holidays etc. I'll request a review and then it should hopefully get merged soon. |
There was a problem hiding this comment.
The code changes look reasonable and the addition of unit tests are consistent.
I could easily reproduce the issue from #98 using an existing Zarr dataset and renaming it to include labels in the directory name. With a build from the HEAD of this repository, BF_CP=target/OMEZarrReader-0.5.3-SNAPSHOT-jar-with-dependencies.jar ~/Documents/GitHub/bioformats/tools/showinf -nopix 03887B26C1_8bit_lynEGFP_with_labels.zarr/.zattrs returns no used files. With this PR included, the command above returns all files. Additionally confirmed that pointing at series group e.g. BF_CP=target/OMEZarrReader-0.5.3-SNAPSHOT-jar-with-dependencies.jar ~/Documents/GitHub/bioformats/tools/showinf -nopix 03887B26C1_8bit_lynEGFP_with_labels.zarr/0/.zattrs works.
I was unable to reproduce #98 . Using the same example as above, I tried to modify the values of channel coefficient or the rendering windows to use either integers or double values. In all cases, the reader was able to initialize the Zarr dataset without exception. Should a specific sample be used for the latter case?
|
Sorry, I can't replicate it, because I can't get a version 0.5.2 of OMEZarrReader with dependencies jar. I leave it up to you, merge or I can also just close the PR again. |
There was a problem hiding this comment.
From my side, I can confirm that this PR addresses 2 out of 3 outstanding issues. I was unable to reproduce the original issue #98 but my tests did not indicate any regression created by the code. The addition of unit tests is certainly a welcome addition that will help future iterations. Despite the absence of a representative sample, I am fine with approving this PR both from a code review and a partial functional testing.
Regarding merging, I have strong reserves with using my write permissions as Glencoe Software is currently not using this reader. While we can certainly help with code review, we are not planning to take onboard the maintenance of this reader.
Related to the point above and as indicated in the description, an impact of the ongoing changes including #97 is that they will require a regeneration of the Bio-Formats memo files. This is known to be a computational exercise for IDR in particular for OME-Zarr datasets. Is there already a roadmap for the software upgrade in order to use the new functionalities described here? /cc @francesw @jburel @will-moore @pwalczysko @khaledk2 @joshmoore.
|
Newer versions of the ZarrReaders have not been included in the openmicroscopy build (see https://github.com/ome/openmicroscopy/blob/develop/etc/omero.properties#L254) |
|
So this change will invalidate ALL Bio-Formats memo files? Or just OME-Zarr memo files? Or just OME-Zarr memo files that are affected by the changes in the PR? Some notes I made on BF cache regeneration previously IDR/idr-metadata#686 |
|
Not the change in this PR, the bump of BF will PR #97, I have reverted the change |
In general, Bio-Formats changes that are backwards-incompatible in terms of the memoization will invalidate all memo files irrespective of the reader. This has been the behavior since OMERO 5. ome/bioformats#4254 is an initial draft proposal that aims to reduce the coupling between all readers and reduce the impact of such changes. As @jburel noted, the upgrade to Bio-Formats 8.x would definitely be memoization breaking. Looking at the diff of these changes alone, I don't see anything that would invalidate a memo file created with ZarrReader 0.5.2 but this should likely be tested to be on the safe side |
|
I don't know why, but I can't replicate the ClassCastException. But it was not only reported by me, also by @jo-mueller (#94) whos fix is included in this PR. I was wondering about the bioformats bump in #97 . At some point we probably have to do it. But according to @will-moore IDR/idr-metadata#686 that's not really an easy task. |
…e conversion properly. * Use `Number` type to handle both `Integer` and `Double`. * Convert `Number` to `Double` using `Number.doubleValue()`.
There a several commits in different PRs I'd need in order to get a ZarrReader I can work with. This PR brings them together:
The commits from #95 and #98 are all included, so these PRs could be closed.
#97 is ready, but still on hold, because it will need memo regeneration once released.