Plugins: Configure slice label name using pattern

[dgault]

This PR is a follow up to https://trello.com/c/KHxM1IIe/14-imagej-imageplus-slicelabel-modification

This adds a new slice label pattern which is stored in LociPrefs
The pattern can be configured using the configuration window. The upgrade tab has been changed to Bio-Formats Options and contains the previous upgrade option as well as the new configuration for the slice label.

The default reproduces the existing default behaviour. Modifying the text field and using Set updates the pattern, using Reset returns to the original default pattern.

The available configurable fields are:
%s - series index
%n - series name
%c - channel index
%w - channel name
%z - Z index
%t - T index
%A - acquisition timestamp

Originally the hope had been to use the FormatTools method for creating a filename as with bfconvert. However a few differences have meant that this was not possible and the existing method has been updated to add similar functionality. The main reasons for this are:

• indexes in the slice label are not zero based
• there is no need to worry about characters such as '/' for file paths or escape chars
• for backwards compatibility indexes only display if dimension size > 1
• due to the previous statement, the dimension prefix is included as part of the index name

[melissalinkert]

Reads fine to me apart from the couple of in-line comments, though I haven't tested thoroughly.

[melissalinkert]

Unit tests are currently failing, as order will be null if retrieve is a DummyMetadata. Using the reader's dimension order instead should work.

[melissalinkert]

Does it make sense to keep a way to include the modulo information in the slice labels?

[sbesson]

A couple of overall comments. Generally, 👍 for making this API more extensible and configurable by the user and reusing the semantics from the command-line tools. In terms of functional testing, I think we want to try various variations of the patterns with various types of images (multi-dimensional, modulo, metadata-rich)

Main immediate concerns are the code duplication between ImagePlusReader.constructSliceLabel() and FormatTools.getFilename and the various flavors of expansion for the index patterns e.g. at the moment %c could be expanded into 0, 000 or c: 1/4 depending on the caller and arguments.

If we go for simple patterns, I wonder whether we can express these expansion rules via a set of options e.g.

pattern.padded: false
pattern.origin: 1 
pattern.prefix: "c: "
pattern.suffix: "/%C"

Also would it make sense to represent the optional aspect of some of the patterns using e.g. [%c][%z][%t]?

[melissalinkert]

It looks like this changes the behavior when square brackets are included in the output filename - is that by design?

With develop alone:

$ bfconvert curated/biorad-gel/samples/imagesEight.1sc "images[8].tiff"
$ ls *.tiff
images[8].tiff

with this PR:

$ bfconvert curated/biorad-gel/samples/imagesEight.1sc "images[8].tiff"
$ ls *.tiff
images8.tiff

Could the new syntax be documented, so that it's clear what the intended/expected behavior is?

[dgault]

Yeah, it still needs to be documented. I had was basing the square brackets on the suggestion from Seb above but it looks like it might not be suitable.

[sbesson]

Agreed. #2891 (comment) convinced me this would be a breaking change for anyone using bracket in their pattern semantics. So probably not a safe change in a minor release.

[dgault]

So to update as to the new additions to getFilename:

%C - channel count
%Z - Z count
%T - T count
[] - optional content when combined with one of the above dimension parameters. Content within the brackets only displays if all dimensions referenced have a size greater than 1

As Melissa has mentioned the use of square brackets for the optional raises issues as valid files use the brackets as part of the filename.

The Travis errors have also unveiled another interesting case of incorrect channel sizes in FormatTools (a previous example was #2848).
In this particular case the Bio-Formats plugin automatically wraps readers with the ChannelSeparator.

The ChannelSeparator will alter the values returned from reader functions such as getEffectiveSizeC, however it never alters the core metadata store.

When using the Bio-Formats plugin there are then calls to retrieve the slice label for each of these separated channels, however as the calculations are based on the core metadata we see the exceptions. The last commit is a rather ugly work a round for this.

[sbesson]

Discussed earlier today at the weekly meeting. The modification of the FormatTools API to support all requirements will require more design and testing notably in terms of pattern expansion and reader/retrieve compatibility. For 5.6.0, the agreement is to limit the scope of this changes to the bio-formats-plugins only. We can schedule the unification of the API discussed in #2891 (comment) in a separate body of work.

[sbesson]

@dgault can you fix the conflicts by merging origin/develop into this branch. We can restart the daily builds to test this PR during the day.

[dgault]

As has been discussed at the formats meeting the last set of commits revert back to original behaviour of the PR in providing an ImageJ specific customisation for the slice label.

[sbesson]

That's fine but there is still a conflict of this PR in components/bio-formats-plugins/src/loci/plugins/util/LociPrefs.java since #2899 got integrated.

[rleigh-codelibre]

With the pattern %c--%z--%t--%n--%w--%A--%s I get odd behaviour with multi-channel-4D-series.ome.tiff: the name is c:1/3 --z:1/5 --t:1/7 --multi-channel-4D-series.ome.tiff--%w-.

%w doesn't seem to be working. If there's no channel name, should it not be blank? Likewise %A; does this prematurely stop processing of the format string if missing?

Other than the above odd behaviour, it see everything seems to work as intended.

Some thoughts: While I can understand the need for backward compatibility, maybe it would be useful to have alternative format strings which are not backward compatible. The output unit appears to be x:m/n . If we have %x to be the current index and %X to be the the total, you could have the default format string written as: c:%c/%C z:%z/%Z t:%t/%T - %n. I don't see the value of having the x: and trailing space in the expansion when they can go directly in the format string. It makes sense to have it all in one unit for backward compatibility, but this could be done using some alternative formatters like %D for c:%c/%C , so that you have the benefit of backward compatibility but the ability to use more flexible alternatives if you don't want that behaviour (both the pre-canned complex format, and/or to also work when the size is 1, for example).

[sbesson]

Tested with Fiji/OSX 10.12, I confirm the issue raised in #2891 (comment) with regard to the expansion of %w which does not currently work.

Otherwise the suggestion from @rleigh-codelibre to define formatters would be a good alternative to handle the ImageJ specificity for label patterns. This would also allow to maintain backwards compatibility, use the existing formatters and minimize the divergence with the FormatTools i.e. the following should work

%D%G%H- %n        # Default ImageJ pattern
%w - z: %c - t %t    # DAPI - z: 1 - t:3
...

[sbesson]

Discussed the last reviews with @dgault. Given the late hour, the proposal would be not to rush this PR into 5.6.0 but take the time to address the issues properly especially given the usefulness of %w. Given the nature of the changes, it should probably be suitable as a patch release candidate. To be rediscussed.

[dgault]

With the latest commits this brings us back to original set of parameters (the same as FormatTools), but with ImageJ specific implementations to maintain the backwards compatibility (this means that if a dimension has a size of 1 it will be skipped).

The available configurable fields are:
%s - series index
%n - series name
%c - channel index
%w - channel name
%z - Z index
%t - T index
%A - acquisition timestamp

To test:
Compare with and without the PR the default slice labels for the following:

• single dimension image
• multi dimension image
• modulo image

Then open the Bio-Formats configuration window:

• Modify the pattern to use each of the available parameters and verify the values displayed are correct
• Test the reset button and verify that images now import with the original default label display

[rleigh-codelibre]

Do we also need %C %Z %T and %S for sizeC, sizeZ, sizeT and channelCount, respectively, so that we can recreate the hardcoded compatibility output with a custom format string?

[mtbc]

Unit test failures in ImporterTest.

[dgault]

@rleigh-codelibre So the extra parameters that had been added earlier in the PR such as sizeC, sizeT, have been removed to keep it inline with the original implementation. The individual dimensions are now expanded to their ImageJ specific format so %c for channel index will become c: 1/4.

This isn't as flexible as the previous 'full' FormatTools solutions but rather to add an initial level of configurability so the user can determined which data to display or hide. The full solution would needs further discussion on the requirements such as dealing with optional parameters.

[sbesson]

Merging this effort to start making progress on the technical documentation front. The extensible slice label feature will be released in the upcoming 5.7.0.

Discussed with @dgault, the only remaining uncertainty from my side is whether re-using the same formatters in the different context of the ImageJ plugin will not create some maintenance issue in the long-run. Hopefully, adding some technical documentation on this feature might help clarifying this, raising blockers missed during the review and having more people assessing at the impact of this change. If this turns out to raise a blocker, we can always define ImageJ-only formatters as suggested above.

[ctrueden]

This is a nice feature, and I appreciate the effort that went into coding it.

However, I do want to caution against using stateful options/preferences, and instead advise providing values to the relevant API itself, e.g. via an instance of ImporterOptions. My big concern here is reproducibility, which is paramount for scientific software such as ImageJ. Every global/stateful option which exists outside of direct API is a potential source of skew/irreproducibility. For a discussion of problems these options cause in practice in ImageJ 1.x, see the options bullet point of this troubleshooting entry.

[dgault]

Thank you for the feedback @ctrueden, that makes sense and is something we should further look into. I have opened a Trello card (https://trello.com/c/P3vIonr0/182-plugins-review-use-of-pref-and-importeroptions) to review our usage and look into potentially switching to ImporterOptions in a follow up to this.