-
Notifications
You must be signed in to change notification settings - Fork 14
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 depth option to importer and include ZarrReader #310
base: master
Are you sure you want to change the base?
Conversation
Hm, maybe the depth option isn't actually required. I can set it to 1 and it still works, weird. |
I think some zarr files might not actually need the option, respectively even a value of 1 will be enough for them. But some don't, so you have to test with a zarr file with a deeper directory structure. |
The import progress display is a bit weird with zarr files. The importer seems to count the files correctly because it switches to the "multi file import display", but then it only shows 1 as file count. I'll try to fix that in the PR too. |
The issue was that Insight was counting files and not images in order to decide if the detailed or "simple" progress display should be used. Hence for zarr (many files / image) that won't work. Should be fixed with the last commit. |
Conflicting PR. Removed from build OMERO-insight-push#1206. See the console output for more details.
|
@dominikl Was this tested via UI? |
Pretty sure I did. But I'll give it another test. |
@pwalczysko is going to test it. It will be good to have a release with that extension following 5.6.5 OMERO.server |
See below from the insight log.
Insight is frozen now. The workflow was:
See |
There's definitely something odd, but it looks like with the ome.tiff files. They take ages for the "Scanning" phase. And as with this PR the scanning also happens when an image is placed into the queue (in order to work out how many files/images to deal with) and not just on import, the whole UI hangs at that point. |
It depends on the file, for example if you try https://downloads.openmicroscopy.org/images/OME-TIFF/2016-06/sub-resolutions/Fluorescence/ both works, ome.tiff and zarr. Should test the other ones which cause problems with older version of Insight. Wonder if it's actually a bioformats issue. |
I have a simplified test. It is enough to concentrate on https://downloads.openmicroscopy.org/images/OME-TIFF/2016-06/BBBC/ . The plate there is called Release insight handles the file successfully (after ca 12 minutes of scanning phase, which is not captured in screenshot, but read it from logs) |
As a side note, making use of ome/ZarrReader#41 during OMERO import (in order to not create bazillions of chunk files in the DB) will likely require detecting a .zarr fileset as something "special" and adding custom behavior. I mention in case it is of use to also bump up the depth setting in this special case. |
Conflicting PR. Removed from build OMERO-insight-push#1297. See the console output for more details.
--conflicts |
Sorry, I can't replicate the issue with Edit: Sorry, I just realised that this PR actually wasn't in the build I tested due to the merge conflict... Will test again, after rebase. |
Conflicting PR. Removed from build OMERO-insight-push#1307. See the console output for more details.
|
Hi, many thanks to this work. Just to mention, if it helps, that I tried an Omero.insight importation on our Omero server (last version I think) of a 327 images z stack after NGFF conversion with NGFF converter from Glencoe and the zar folder was imported. In omero.web 5.13.0 the name of the file was changed to METADATA.ome.xml the image properties seem to be kept but the image is dark, and also in omero.viewer whatever the contrast adjustments. I could try things for you if needed. |
Thanks for the feedback, @brau-frederic! The darkness is likely an issue of there not being a rendering model in OME-XML and therefore no way for Bio-Formats to hand that to OMERO. cc: @dgault |
@dominikl @pwalczysko What is the status of this PR? I will prepare a release of omero.insight after the openmicroscopy release |
@@ -671,6 +677,13 @@ public void actionPerformed(ActionEvent e) { | |||
tagsMap = new LinkedHashMap<JButton, TagAnnotationData>(); | |||
mapAnnotation=new LinkedHashMap<String,List<MapAnnotationData>>(); | |||
|
|||
depth = new NumericalTextField(); | |||
depth.setMinimum(1); | |||
depth.setText(System.getProperty("omero.import.depth", "50")); |
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 property should be put in the config or retrieve from the server if available.
@@ -225,6 +222,9 @@ public static boolean isArbitraryFile(File f) | |||
/** Map of skip options mapping to ImportConfig functions */ | |||
private Map<String, Object> skipChoices; | |||
|
|||
/** The depth Bioformats uses to find the importable files */ | |||
private int depth = Integer.valueOf(System.getProperty("omero.import.depth", "4")); |
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.
Same
I couldn't replicate the difference with/without PR with this specific |
Thanks @dominikl. I don't think we can consider this PR for next release. |
--exclude |
This PR adds an additional "depth" option to the importer (with default value 50) and adds the ZarrReader (cherry-picked from #309 , thanks @jburel !).
Test: Import an OME-NGFF Zarr file.