Add extended subresolution support #3119

rleigh-codelibre · 2018-04-03T09:49:12Z

Intended for improved subresolution support in readers, for adding SUBIFDS support to TIFF and OME-TIFF readers.

Add MetadataList and CoreMetadataList classes and unit tests
Add SubResolutionFormatReader class as an alternative to FormatReader, using CoreMetadataList
Deprecate core-index-related methods in IFormatReader, which are exposing internal implementation details of FormatReader
Readers for sub-resolution formats and all TIFF formats use SubResolutionFormatReader
Note the subclassing of CoreMetadata in SVSReader to store reader-specific subresolution-specific metadata directly in the CoreMetadataList container

CoreMetadataList replaces the ArrayList<CoreMetadata> in FormatReader, in a new SubResolutionFormatReader class (to avoid any API breaks in FormatReader). The difference is that it's an array of arrays, representing series and resolution, rather than a flat list. This makes lookups O(1) rather than O(n) due to avoiding the need to do a full traversal or series of hops over the list; it's now always two array lookups. It implements the bits of ArrayList needed to be as compatible as possible with the previous implementation, so it's a drop-in replacement in many (but not all) aspects. SubResolutionFormatReader is otherwise a complete duplication of the existing FormatReader.

MetadataList is a generalisation of CoreMetadataList to be used for storing arbitrary metadata by individual readers such as IFD mappings, or whatever, for series and subresolutions. Currently used as the underlying implementation of CoreMetadataList, but will be used by the TIFF and other readers in the future. This could be used to simplify some of the existing readers providing subresolutions, as well as to provide new capabilities, such as SUBIFDS support in the TIFF readers. If you don't subclass CoreMetadata, this allows separate containers for different types of reader-specific metadata.

Testing:

Ensure all builds are green and all full repository tests are green (reader regressions are possible)
Pay special attention to these formats (check that the series and resolutions are in the same order with both flat and nonflat viewing modes, and that the pixel data is OK):
- AFI (SVS-derived)
- Imaris HDF
- Leica-SCN
- OMERO Pyramid TIFF
- SVS

(the full repo test should verify all this, but it wouldn't hurt to check over a random sampling).

rleigh-codelibre · 2018-04-03T22:38:09Z

@sbesson Looks like there's a handful of failures in https://web-proxy.openmicroscopy.org/east-ci/job/BIOFORMATS-test-folder/5669/console . I'm not familiar with this new test setup; it looks like several TIFF-based formats are being tested here, but I'm not sure what the commonality is. Is this multiple formats?

mtbc · 2018-04-04T07:17:54Z

components/formats-api/src/loci/formats/MetadataList.java

+ * @param <T> The type of metadata to store.
+ */
+public class MetadataList<T> {
+ protected ArrayList<ArrayList<T>> data = new ArrayList<>();


Is it okay to declare data as being simply List<List<T>>?

Annoyingly, it doesn't work. Nested generics cause javac to error out. I tried and failed, and also worked through several ways to do it with Riad, without success.

I'll fetch your branch and take a look.

diff --git a/components/formats-api/src/loci/formats/CoreMetadataList.java b/components/formats-api/src/loci/formats/CoreMetadataList.java index b0b84ab076..6b8d627f24 100644 --- a/components/formats-api/src/loci/formats/CoreMetadataList.java +++ b/components/formats-api/src/loci/formats/CoreMetadataList.java @@ -105,7 +105,7 @@ public class CoreMetadataList extends MetadataList<CoreMetadata> { } public void reorder() { - for (ArrayList<CoreMetadata> s : data) { + for (List<CoreMetadata> s : data) { Collections.sort(s, comparator); } } diff --git a/components/formats-api/src/loci/formats/MetadataList.java b/components/formats-api/src/loci/formats/MetadataList.java index 5fa7215b50..b1a09f9ac3 100644 --- a/components/formats-api/src/loci/formats/MetadataList.java +++ b/components/formats-api/src/loci/formats/MetadataList.java @@ -52,7 +52,7 @@ import java.util.ArrayList; * @param <T> The type of metadata to store. */ public class MetadataList<T> { - protected ArrayList<ArrayList<T>> data = new ArrayList<>(); + protected List<List<T>> data = new ArrayList<>(); // Construct an empty list. public MetadataList() {

Does this not suffice? It builds cleanly for me.

mtbc · 2018-04-04T07:21:22Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ /**
+ * Initializes a reader from the input file name.
+ *
+ * Calls {@link #initFile(String id)} to initializes the input file, reads


initialize, etc. - some odd tense agreement issue in this sentence

Yes, these are inherited directly from the FormatReader javadocs. I'll fix both up.

My issue was with the sentence starting where this comment is.

Ah, sorry, fixed this now.

mtbc · 2018-04-04T07:23:04Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ String name = "Series " + series;
+ try {
+ String realName = ((IMetadata) store).getImageName(series);
+ if (realName != null && realName.trim().length() != 0) {


This looks very much like StringUtils.isNotBlank(realName) rather like https://github.com/openmicroscopy/bioformats/blob/v5.8.1/components/formats-gpl/src/loci/formats/in/LIFReader.java#L2040.

I think you're right. However, we don't currently have any apache-commons dependencies in formats-api, so I'm not sure if it's worth adding for the sake of one line, which was already in this form in FormatReader.

sbesson · 2018-04-04T08:11:01Z

@rleigh-codelibre each BIOFORMATs-test-folder job runs against one top-level folder under the curated data repository. Recent top-level folders match a given file format but this rule does not apply across the board. The error above seemed to be transient and was fixed by a re-run against the zeiss-tiff subdirectory (see log)

Probably, the first issue to resolve in order to test this against the data repository is to fix the Docker image build logic. Consuming the east integration branch leads to error when retrieving SNAPSHOT dependencies - see also ome/bio-formats-build#11.

rleigh-codelibre · 2018-04-04T15:00:04Z

First test run through is https://web-proxy.openmicroscopy.org/east-ci/job/BIOFORMATS-test-repo/44/flowGraphTable/

rleigh-codelibre · 2018-04-04T21:30:04Z

Closing temporarily so we can see if tomorrow's build still has failures; may be some configuration issues with the new docker image and/or the repo config being used which are independent of this PR and should be identified and fixed separately.

rleigh-codelibre · 2018-04-07T08:28:22Z

Reopen now https://web-proxy.openmicroscopy.org/east-ci/job/BIOFORMATS-test-repo/53/ is green again.

mtbc · 2018-04-16T09:32:24Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ protected int resolution = 0;
+
+ /**
+ *


mtbc · 2018-04-16T09:36:00Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+
+ /** Adds an entry to the global metadata table. */
+ protected void addGlobalMeta(String key, boolean value) {
+ addGlobalMeta(key, new Boolean(value));


The static valueOf methods are the ones to use here and below.

mtbc · 2018-04-16T09:37:55Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ protected void addMetaList(String key, Object value,
+ Hashtable<String, Object> meta)
+ {
+ Vector list = (Vector) meta.get(key);


No need to get, the remove gives it to us. (Also below.)

mtbc · 2018-04-16T09:39:31Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+
+ /** Adds an entry to the metadata table for the current series. */
+ protected void addSeriesMeta(String key, boolean value) {
+ addSeriesMeta(key, new Boolean(value));


mtbc · 2018-04-16T09:42:49Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ }
+ catch (IllegalArgumentException e) {
+ throw new FormatException("Image plane too large. Only 2GB of data can " +
+ "be extracted at one time. You can workaround the problem by opening " +


work around

mtbc · 2018-04-16T09:48:35Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+
+ if (hasFlattenedResolutions()) {
+ return 1;
+ }


close up linebreak to } else { (also below)

While I'd like this, it's actually the other way around in terms of code style!

Ah, fair enough, another OMERO/BF style difference. Certainly keeps me on top of my options dialogs in IDEs!

mtbc · 2018-04-16T09:49:19Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ /* @see IFormatHandler#setCoreIndex(int) */
+ @Override
+ public void setCoreIndex(int no) {
+


remove blank line

mtbc · 2018-04-16T09:53:25Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ setupService();
+ Hashtable<String, Object> allMetadata =
+ new Hashtable<String, Object>();
+ allMetadata.putAll(metadata);


There's a copy constructor you can use instead.

mtbc · 2018-04-16T09:58:46Z

components/formats-gpl/src/loci/formats/in/LeicaSCNReader.java

-import java.util.ArrayList;
-import java.util.Deque;
-import java.util.HashMap;
+import java.util.*;


We usually try to avoid wildcard imports.

mtbc · 2018-04-16T09:59:07Z

components/formats-gpl/src/loci/formats/in/LeicaSCNReader.java


 if (c == null || i == null) {
- throw new FormatException("Error setting core metadata for image number " + s);
+ throw new FormatException("Error setting core metadata for series " + series + " resolution" + resolution);


space or = before resolution

mtbc · 2018-04-16T10:02:04Z

components/formats-gpl/src/loci/formats/in/MetamorphReader.java

@@ -42,6 +42,7 @@
 import java.util.regex.Pattern;
 import java.util.regex.Matcher;

+import loci.formats.*;


Try to avoid wildcard imports.

mtbc · 2018-04-16T10:03:10Z

components/formats-gpl/src/loci/formats/in/NDPIReader.java

+ ms.pixelType = ifd.getPixelType();
+ ms.metadataComplete = true;
+ ms.interleaved =
+ ms.sizeX > MAX_SIZE && ms.sizeY > MAX_SIZE;


Interesting it's a && but outwith this PR.

mtbc · 2018-04-16T10:03:58Z

components/formats-gpl/src/loci/formats/in/NativeND2Reader.java

-import loci.formats.FormatTools;
-import loci.formats.ImageTools;
-import loci.formats.MetadataTools;
+import loci.formats.*;


wildcard again

sbesson · 2018-04-17T07:12:58Z

I have not reviewed the code yet but assuming the repository tests have been passing with these changes and there are no blockers found by the reviewers, my vote would be in favor of merging this into the east integration branch and capturing all outstanding comments and questions (cleanups, reader, API...) as a 5.9.0 card so that they can be addressed in follow-up PRs. A final review should happen when the east integration branch is proposed for the integration into develop.

mtbc · 2018-04-17T07:18:08Z

No blockers from me.

joshmoore

A few specific comments followed by:

Depending on the List<T> discussion, we should come back to the name of the new format reader class.
Also depending on that discussion, I could imagine maintaining a get(0) which calls out to get(0, 0) to reduce some of the changes here.

Leaving the PR open for a few more days for anyone to review the API, but I agree with @sbesson: I don't foresee us getting all the comments made on this PR done with a single merge. Happy to have east used as the integration branch.

joshmoore · 2018-04-17T14:48:17Z

components/formats-api/src/loci/formats/MetadataList.java

+ * @param <T> The type of metadata to store.
+ */
+public class MetadataList<T> {
+ protected List<List<T>> data = new ArrayList<>();


The previous work on core metadata moved from a T[] to a List<T>. This moves to a List<List<T>>. Either I think that definition should be at the heart of the specific FormatReader class, i.e. when one is implementing a reader one chooses the CoreMetadataListFormatReader or the CoreMetadataListListFormatReader or we should think about future-proofing a bit. e.g. what would it mean to add a third element here?

It wouldn't mean anything to add a third element right now. The split is series/resolution, and this will be sufficiently future-proof for subresolutions in z.

If we want to get more complex, I think we would have to move to an entirely new interface. This is all an artefact of being backward compatible with the old FormatReader API when adding subresolutions; ideally we would remove most of the coreIndex-related complexity entirely, but that's not currently an option.

joshmoore · 2018-04-17T14:50:23Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ // -- Constants --
+
+ /** Default thumbnail width and height. */
+ protected static final int THUMBNAIL_DIMENSION = 128;


Some of these static fields make me wonder if we need to introduce some common element (via inheritance or composition) so the values aren't cut-n-pasted. At the same time, this new class may be a chance to move them away from protected in favor of private.

Anything common should probably go into FormatTools.

joshmoore · 2018-04-17T14:59:51Z

components/formats-api/src/loci/formats/SubResolutionFormatReader.java

+ /* @see IFormatReader#getOptimalTileHeight() */
+ @Override
+ public int getOptimalTileHeight() {
+ FormatTools.assertId(currentId, true, 1);


Similar to the constants above, the functionality in this class should be refactored elsewhere.

joshmoore · 2018-04-17T15:07:46Z

components/formats-gpl/src/loci/formats/in/SVSReader.java

@@ -52,6 +49,16 @@
 import ome.units.quantity.Length;
 import ome.units.UNITS;

+class SVSCoreMetadata extends CoreMetadata {


Is access needed to these fields or can this be private static?

It should be package-private already?

joshmoore · 2018-04-17T15:09:03Z

components/formats-gpl/src/loci/formats/in/SVSReader.java

@@ -64,16 +71,12 @@
 LoggerFactory.getLogger(SVSReader.class);

 /** TIFF image description prefix for Aperio SVS files. */
- public static final String APERIO_IMAGE_DESCRIPTION_PREFIX = "Aperio Image";
+ private static final String APERIO_IMAGE_DESCRIPTION_PREFIX = "Aperio Image";


Not that I don't approve of more privates, but what's the reasoning behind this change? Do we know there are no consumers?

It's a static string for a reader implementation with no reason for anyone to be using it. There might be a "consumer", but it's most unlikely. This should never have been public. I can revert it if required.

I don't know how we could know there are no consumers? But could at least deprecate now if we'd like to get rid of this.

mtbc · 2018-04-25T13:49:39Z

components/formats-api/src/loci/formats/FormatReader.java

@@ -878,7 +878,7 @@ public boolean isInterleaved(int subC) {
 }
 catch (IllegalArgumentException e) {
 throw new FormatException("Image plane too large. Only 2GB of data can " +
- "be extracted at one time. You can workaround the problem by opening " +
+ "be extracted at one time. You can worka round the problem by opening " +


Argh. Fixed now, thanks.

…ng the series

…ss setting the series" This reverts commit b877f07. The logic in FileStitcher looks a bit inconsistent, so leave well alone for now.

rleigh-codelibre · 2018-04-25T14:08:44Z

Rebased onto current east following the merge of 5.8.2.

rleigh-codelibre · 2018-04-26T18:39:45Z

@sbesson Green in https://web-proxy.openmicroscopy.org/east-ci/job/BIOFORMATS-test-repo/78/

joshmoore · 2018-04-30T15:24:14Z

With the rebasing, is there any capture of which suggestions were made here that have not yet been addressed?

rleigh-codelibre · 2018-04-30T15:31:03Z

I don't think the rebase lost any suggestions (I was pleasantly surprised, must be a recent change). Every suggestion has been fixed or commented on.

The main outstanding item would be your suggestion to factor out common code from the two implementations. I've already done so for a lot of the convenience methods (into MetadataTools). The remaining things can be factored out as needed.

mtbc reviewed Apr 4, 2018

View reviewed changes

rleigh-codelibre closed this Apr 4, 2018

sbesson mentioned this pull request Apr 6, 2018

test-suite: Add testng.allow-missing property #3122

Merged

rleigh-codelibre reopened this Apr 7, 2018

mtbc reviewed Apr 16, 2018

View reviewed changes

components/formats-api/src/loci/formats/SubResolutionFormatReader.java Outdated

protected int resolution = 0;

/**

*

Copy link

Member

mtbc Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

mtbc reviewed Apr 16, 2018

View reviewed changes

dgault mentioned this pull request Apr 17, 2018

High memory consumption #3124

Open

joshmoore reviewed Apr 17, 2018

View reviewed changes

mtbc reviewed Apr 25, 2018

View reviewed changes

formats-api: Add MetadataList and CoreMetadataList

80ec8a6

rleigh-codelibre and others added 22 commits April 25, 2018 14:51

formats-api: *FormatReader: Correct Javadoc grammar (2)

a2eac14

test-suite: FormatReaderTest: Increase message verbosity

800ca9f

test-suite: FormatReaderTest: Add additional assertions

4bc3db0

formats-bsd: FileStitcher: Don't reset reader resolution unless setti…

c13baba

…ng the series

Revert "formats-bsd: FileStitcher: Don't reset reader resolution unle…

0bd89c3

…ss setting the series" This reverts commit b877f07. The logic in FileStitcher looks a bit inconsistent, so leave well alone for now.

SubResolutionFormatReader: Drop transform rotation creation method

f6229f0

MetadataTools: Add enum convenience methods

377d2b3

SubResolutionFormatReader: Drop enum convenience methods

f57cd0e

gitignore: Ignore IntelliJ IDEA files

cb0a5cb

*FormatReader: Use .valueOf when creating numeric types

36e2bd7

SubResolutionFormatReader: Add comment for core list

10719fd

*FormatReader: Correct spelling

331f789

SubResolutionFormatReader: Use consistent else braces

55385a3

SubResolutionFormatReader: Remove redundant blank line in setCoreIndex

833c0b1

*FormatReader: Use Hashtable copy constructor

d33a12a

LeicaSCNReader: Restore individual imports

e6c2ead

LeicaSCNReader: Add missing space

f8efd60

MetamorphReader: Restore individual imports

727a4ad

NativeND2Reader: Restore individual imports

d98110f

ND2Handler: Restore individual imports

4b4a9f3

MinimalTiffReader: Restore individual imports

dd4b9a1

*FormatReader: addMetaList uses remove only

6f972ae

sbesson merged commit 2daf7c6 into ome:east Apr 26, 2018

rleigh-codelibre deleted the series-metadata branch April 26, 2018 19:43

rleigh-codelibre mentioned this pull request May 2, 2018

OMETiffReader extends SubResolutionFormatReader #3136

Merged

sbesson mentioned this pull request Sep 18, 2018

OME-TIFF pyramid: reading and writing #3223

Merged

Add extended subresolution support #3119

Add extended subresolution support #3119

Conversation

rleigh-codelibre commented Apr 3, 2018 • edited Loading

rleigh-codelibre commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbc Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbc Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson commented Apr 4, 2018

rleigh-codelibre commented Apr 4, 2018

rleigh-codelibre commented Apr 4, 2018 • edited Loading

rleigh-codelibre commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbc Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbc Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbc Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

mtbc Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson commented Apr 17, 2018

mtbc commented Apr 17, 2018

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleigh-codelibre commented Apr 25, 2018

rleigh-codelibre commented Apr 26, 2018

joshmoore commented Apr 30, 2018

rleigh-codelibre commented Apr 30, 2018

rleigh-codelibre commented Apr 3, 2018 •

edited

Loading

mtbc Apr 4, 2018 •

edited

Loading

mtbc Apr 4, 2018 •

edited

Loading

rleigh-codelibre commented Apr 4, 2018 •

edited

Loading

mtbc Apr 16, 2018 •

edited

Loading

mtbc Apr 16, 2018 •

edited

Loading

mtbc Apr 16, 2018 •

edited

Loading

mtbc Apr 16, 2018 •

edited

Loading