Support min/max to update StatsInfo

[will-moore]

This adds support for setting min and max.
E.g:

---
channels:
    1:
        active: true
        end: 2000.0
        start: 150.0
        min: 10
        max: 3000
greyscale: false
version: 2

Both min and max are optional and don't depend on each other.
To test:

• omero render set Image:1 settings.yml
• Then Refresh right panel in webclient, or refresh iviewer and click Min/Max to set rendering settings to the new min/max values.

cc @pwalczysko

Based on example code from:
https://github.com/ome/omero-scripts/pull/103/files#diff-70c904f67c4f0348841edf1ebdac7c32d8914ab87ac05a0a8f9948d47a8b94d7R195

[snoopycrimecop]

Conflicting PR. Removed from build OMERO-plugins-push#640. See the console output for more details.
Possible conflicts:

• PR Add batchset for rendering a folder of datasets #50 manics 'Add batchset for rendering a folder of datasets'
  • src/omero_cli_render.py
• PR test command: also print image id #51 dominikl 'test command: also print image id'
  • src/omero_cli_render.py
• PR Add batchset for rendering a hierarchy of containers (single file) #52 manics ' Add batchset for rendering a hierarchy of containers (single file)'
  • src/omero_cli_render.py

--conflicts Conflict resolved in build OMERO-plugins-push#641. See the console output for more details.

[joshmoore]

Would one ever want to null the value that's in the stats info? If so, this continue would prevent that, right?

[will-moore]

Yes, OK. So if I put min: null into the yaml then this line fails self.min = float(d['min']) if 'min' in d else None.
I could work around that, but I'd need to set self.min to some value that indicates 'unset', since None is already used to mean "Don't change anything".
In JavaScript we have undefined and null to choose from, but Python only has None!

[sbesson]

Thinking of the YAML contract from a round-tripping perspective, what it the behavior of omero render info for an Image for which the min/max has been nulled in stats info? Does it default to the full pixel type range?

[will-moore]

@sbesson Yes. E.g. $ omero render info Image:9621401 for IDR ...min=0.0,max=65535.0 for all channels.

[pwalczysko]

Works as expected. Tested

• pulling the rnd from an image into a rnd.yml file and re-applying the same or adjusted rnd.yml settings back onto that image
• testing on 3 different types of images, including nd2 and floating point images and timelapses
• testing on P, D, I
• leaving min and max undefined (deleting the whole line from the yml file)

Ready to merge fmpov

[sbesson]

Thanks for opening this change. I am all for getting this quickly into a release of the render plugin. In terms of feature completion, all fields exported by the info command should now be modifiable. This is something that should be directly solving some use cases like the one we saw in IDR.

One request from my side: would it possible to add at lest one integration test to test_render.py covering the min/max scenario to prevent regressions in the future?

[will-moore]

Any ideas for how to unset a min/max value as Josh was suggesting?
Or maybe this is enough of an edge case to leave for now (until we actually need it) and we don't want to change the behaviour of def init_from_dict() to set self.min = float or None or some-other-value-that-means-unset?

I'll try to add a test ASAP.

[will-moore]

Test added.
I noticed from the tests that you can't have min or max set if the other is unset (if you have StatsInfo it needs both).
So any unset would fail unless it deleted the StatsInfo to remove both.

[joshmoore]

"if set" ?

[will-moore]

I wanted to say (need to set max if max is unset) but line length too long

[sbesson]

What about something like

        min: <float>        Min pixel intensity, required if max is set
        max: <float>        Max pixel intensity, required if min is set

to communicate the conditional requirement?

[will-moore]

What I'm trying to say here is that if the channel has no StatsInfo then you need to set min AND max. This is the same as setting start/end, where we use the format:
start: <float> Start of rendering window, optional (needs end)

But if the channel already has StatsInfo the you can set min or max independently.

I don't know if it's important to document this 2nd case. I don't think it's possible to explain that in the one-line we have here, so maybe we should just be consistent and do:
max: <float> Maximum pixel intensity, optional (needs min)

[sbesson]

Understood and I certainly missed the subtle conditional behavior.

I think it's too much to try and describe in such a short field description, maybe we just mention , optional and have a dedicated text paragraph below to expand on the two use cases i.e. whether stats info is null or not. Is there a way to know whether stats info is null?

[sbesson]

Thanks @will-moore for the tests and explanation. Retested against UoD CI servers using a sample file from the original IDR submission which triggered this feature addition. Everything behaves as expected with the min/max being now fully readable/writable via omero render info/set. The new integration test also adds coverage for the new functionality as requested earlier.

From the PR discussion, the only remaining question is whether we want to support nullifying min/max in StatsInfo. From #53 (comment), I am inclined to say this requires more thoughts and potentially a scenario where one would like to null existing min/max values. My understanding is that additional API and format specification might be required to differentiate the cases of min/max values either set to null or explicitly set to the limit of the pixel type range.

Immediately, I would propose to get this merged and released. In terms of contract, no new API is introduced in this PR but I feel this is rather implementing a missing functionality rather than fixing a bug, so I'd propose 0.7.0. Objections?

[joshmoore]

WFM.