.vmic WSI reader - #4397
Conversation
|
@sbesson @melissalinkert following up on Melissa's note connecting this to #4453. Kai and I have agreed to consolidate onto #4397 (mine adds a synthetic-fixture unit test and a few robustness fixes on top of his reader), and since it's milestoned for 9.0.0 I wanted to ask whether it could get a first review. That would tell us what to change before I open the follow-up against Kai's branch, and it avoids duplicating effort. No urgency on merging. Thanks! |
melissalinkert
left a comment
There was a problem hiding this comment.
Thanks @kwiechen and @kasper-cancilico for reviving this discussion, and apologies for the delay.
Please see initial inline review comments; also please revert the move of SubResolutionExample.java.
In places where there is some unexpected/undesired behavior, I've tried to note commands you can run on your own to reproduce that behavior. Another test you can run is to take the files from https://zenodo.org/records/18241389, put them in a directory, and then run (from the root of this repo):
$ cd components/test-suite
$ ant -Dtestng.memory=2g -Dtestng.directory=/path/to/vmic/data -Dtestng.configDirectory=/path/to/vmic/data test-config
$ ant -Dtestng.memory=2g -Dtestng.directory=/path/to/vmic/data -Dtestng.configDirectory=/path/to/vmic/data test-automated
See https://bio-formats.readthedocs.io/en/latest/developers/commit-testing.html#automated-tests for details. That is what we would do to get data into our test repo, so a prerequisite for merging is that both of those ant ... commands run without errors.
Finally, if you are planning to add commits here, @kasper-cancilico, we would need a signed Contributor License Agreement (we already have one for @kwiechen).
|
|
||
| public VmicReader() { | ||
| super("vmic", new String[]{"vmic"}); | ||
| domains = new String[]{FormatTools.GRAPHICS_DOMAIN}; |
There was a problem hiding this comment.
Since this is whole slide data, I'd expect FormatTools.HISTOLOGY_DOMAIN here.
| import loci.formats.meta.MetadataStore; | ||
| import ome.units.UNITS; | ||
| import ome.units.quantity.Length; | ||
| import org.w3c.dom.*; |
There was a problem hiding this comment.
Here and in other imports above, we prefer to have necessary imports enumerated instead of using *.
| Files.delete(tempFileToDelete); | ||
| } | ||
| catch (IOException e) { | ||
| System.out.println(e.getMessage()); |
There was a problem hiding this comment.
Please use the existing logging infrastructure instead of System.out.println.
| public void close(boolean fileOnly) throws IOException { | ||
| super.close(fileOnly); | ||
|
|
||
| if (inner_zipfs != null) { |
There was a problem hiding this comment.
If close(false) is called, all fields should be reset to their original value, including e.g. filesFolder. I would expect that lines 145-158 should only happen if close(false) was called (and not close(true)), but if there is a reason that this is always done then it would be helpful to clarify.
|
|
||
| /* @see IFormatReader#getResolutionCount() */ | ||
| @Override | ||
| public int getResolutionCount() { |
There was a problem hiding this comment.
Overriding getResolutionCount() should be unnecessary.
I note that showinf -nopix -noflat vmic-sample-small-1.vmic shows a single pyramid of 6 resolutions. I would then expect showinf -nopix vmic-sample-small-1.vmic to show 6 separate series (one resolution per series), but instead I see a single series.
| store.setInstrumentID("PreciPoint", 0); | ||
| store.setObjectiveSettingsID(metaDataMap.get("ShortName"), 0); | ||
| store.setObjectiveID(metaDataMap.get("ShortName"), 0, 0); | ||
| store.setObjectiveNominalMagnification(Double.parseDouble(metaDataMap.get("Magnification")), 0, 0); |
There was a problem hiding this comment.
As with the PixelPerMicron key, the value for Magnification could possibly be null.
|
|
||
| if (Files.exists(entry)) { | ||
| InputStream is = Files.newInputStream(entry); | ||
| try (ImageInputStream iis = ImageIO.createImageInputStream(is)) { |
There was a problem hiding this comment.
Is there a reason to re-implement ImageIO logic here, instead of making use of existing Bio-Formats readers that use ImageIO (e.g. JPEGReader)? That would also allow removal of most of the BufferedImage handling, since the existing ImageIO-based readers already unpack BufferedImage to a byte array.
| private static final String EXTENDED_METADATA = "VMCF/config.osc"; | ||
| public static final String DZI_FILES = "dzc_output_files"; | ||
|
|
||
| private File filesFolder; |
There was a problem hiding this comment.
File variables will break memoization, which showinf -nopix -noflat -cache /path/to/file.vmic can demonstrate. For filesFolder, it should be enough to remove this line, and replace use of filesFolder with DZI_FILES.
| private int maxLevel; | ||
|
|
||
| private FileSystem inner_zipfs; | ||
| private File innerZipFile; |
There was a problem hiding this comment.
Similar to filesFolder above, the File variable is a problem for memoization. If innerZipFile is needed outside of initFile, you could store the String path instead.
| private int height; | ||
| private int maxLevel; | ||
|
|
||
| private FileSystem inner_zipfs; |
There was a problem hiding this comment.
This will also cause a problem for memoization, but is a little less straightforward to fix. The most consistent with other readers would be to make this field transient, then override the reopenFile() method to set inner_zipfs appropriately. ZipReader and MinimalTiffReader are two examples that follow a similar pattern.
Hello,
the .vmic WSI file (#4264) is a nested Zip archive including a deep zoom pyramid. .vmic format information is from:
openslide/openslide#168
https://lists.andrew.cmu.edu/pipermail/openslide-users/2015-December/001164.html
The VmicReader reads the deep zoom pyramid using:
https://github.com/usnistgov/pyramidio
Reading data from WSI files < 2 GB is possible directly using java.nio.file.FileSystems. The reader unzips the outer zip container into a temporary file if size is > 2 GB.
Example data: https://zenodo.org/records/14714280
Best regards,
Kai