Codomain web

[will-moore]

What this PR does

Webclient support for 'Reverse Map Codomains' which allows you to reverse the intensity rendering for each channel in an image.
Based on server-side work in #4823.

Testing this PR

1. Test at http://web-dev-merge.openmicroscopy.org/webclient/
2. Click on the color-picker button for a channel, reverse intensity checkbox appears at top of list.
3. Reverse intensity should be applied when checkbox is selected. 'Save' should become enabled. Checkbox should always represent the current 'reverse' status (when colorpicker is launched for each channel)
4. Full testing of rendering editing, undo/redo, copying, pasting, applying 'owners', user's & imported in Preview Panel and Image Viewer.
5. Also test chgrp of an image with Reverse Intensity saved.

Related reading

https://trello.com/c/9iAe3Sg9/95-lut-support

[snoopycrimecop]

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

• PR histograms #4703 will-moore 'histograms'
  • components/tools/OmeroWeb/omeroweb/webgateway/static/webgateway/js/omero_image.js

[snoopycrimecop]

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

• PR histograms #4703 will-moore 'histograms'
  • components/tools/OmeroWeb/omeroweb/webgateway/static/webgateway/js/omero_image.js

[joshmoore]

I've added --exclude to the description so that this doesn't keep conflicting.

[jburel]

Better to exclude the histogram PR from the build since it has been marked as onhold.
I have now changed the marker i.e. remove exclude from this PR, and excluded the histogram one

[dominikl]

Looks alright! 👍

[will-moore]

@chris-allan You might want to look at the BlitzGateway and webgateway parts of this PR.
Especially the addition to the rendering query string. To reverse the mapping, I've added an 'r' flag after the start:stop window. E.g. to reverse just the first channel: c=1|0:255r$FF0000,2|0:255$00FF00.
The logic is currently to turn off Reverse Codomain Map if the 'r' is missing (even if the image has Reverse Map saved). However, I wonder if missing 'r' should "do nothing" and show whatever Reverse Map is saved on the image?
E.g. If you saved a Reverse Map on a channel in Insight/webclient, then added the image to OMERO.figure (or some other app that didn't know about Reverse Map), I guess we should still show the 'default' / saved state.
In that case, we'd need a different flag to remove the Reverse Map if it is saved. E.g. '-r', c=1|0:255r$FF0000,2|0:255-r$00FF00. Thoughts?

[jburel]

if we re-activate other codomain map e.g. plane slicing or contrast stretching
what will be the strategy in that case @will-moore?

[will-moore]

@jburel The strategy will always be to find a simple, clear and concise way of encoding that info into a query string. It depends on what extra parameters we need for plane slicing or contrast stretching. For reverse it is really just a boolean flag per channel so it is quite simple.

[will-moore]

To test the last commits, save Reverse Intensity on a channel in webclient then import the image into OMERO.figure. The channel should still be rendered with Reverse Intensity.

Also test chgrp of an image with Reverse Intensity saved.

[pwalczysko]

RFE: Fix the "Reverse intensity" checkbox line so that when scrolling down to the lower-listed LUTs the "Reverse intensity" is still clickable.

[pwalczysko]

Bug: Save to All does not propagate "switch off" of the "Reverse" checkbox.

• go to a plate
• on some well, check the "Reverse" checkbox in one or more channels
• press Save To All -> action works as expected
• now go to another well and UNCHECK the "Reverse" checkbox on one or more channels
• press Save To All -> observe that the action works fine only on the well you are now at, the other wells have all the other changes propagated to them EXCEPT the UNCHECK action

For the images in Dataset there is a similar bug with workflow:

• have 2 images in a Dataset with "Reverse" checkbox ticked on
• uncheck "Reverse" checkox in one of the images and Copy the Rnd settings
• go to the Dataset and right-click Paste and Save the Rnd settings
• observe that the second image does NOT have the "Reverse" checkbox unchecked -> not expected

[will-moore]

@waxenegger Thanks. I don't have a strong opinion on how to encode this and I'm happy to go with your suggestion, but I'd prefer not to have to change it too many times.

@chris-allan Do you have any opinion on this. E.g. encode reverse intensity ON :r (first channel) or OFF :-r (second channel) like this

`````` ?c=1|0:255:r$FF0000,2|0:255:-r$00FF00```

And if it's missing (E.g. ?c=1|0:255$FF0000,2|0:255$00FF00 ) then we render according to the saved codomain reverse map for each channel. Sound OK?

[chris-allan]

I'm a bit torn to be honest. Ideally we would say something like r=true but that's not really feasible. I think we would want to stay away from adding more positional dependency into those query string arguments.

The r vs. -r separated by a final : seems like a good idea with the caveat that we might add additional "flags" to rendering in the future. Would separating these flags into a | separated and then consequently : separated section make sense? This would allow for the following pseudo-grammar:

<channel_number>|<input_start>:<input_end>$<hex_color>[|][flag:flag ...]

Using your example you would then have something like:

c=1|0:255$FF0000|r,2|0:255$00FF00|-r

The second channel could omit the trailing |-r utilising default to OFF semantics.

[waxenegger]

separation by | is a decent option too.

only thing worth mentioning perhaps is that if further parameters are added with pipes, the optional nature of them needs to be addressed. either only the last one is optional or a distinguishing feature is needed to know which is which. with letters that might not constitute a problem, if numbers alone are added it becomes more of an issue.

personally I don't mind either, separation by : or appending with | because they are both easy to parse/dissect.

[chris-allan]

Agreed, @waxenegger.

I think we would have to explicitly state if further | separated parameters are added then they cannot be optional. Positionally that would be a nightmare. In fact I would almost go so far as to say if we need to add additional | separated parameters that we deprecate this entire URL style and redesign it. It was not built with extensibility of this manner in mind.

If you want to go with a completely backwards, and forward, compatible option @will-moore, maybe we should just use an additional r=<channel_number>,[channel_number, ...] query string argument to denote which channels need reverse intensity applied? There's no crazy separator parsing then.

[will-moore]

@chris-allan We need a way to both state that a channel is reversed AND to state that it is not reversed. We can't just have it defaulting to OFF if there's no flag. The default will be the saved state (E.g. as we do now with colors etc). E.g. If you just use ?c=1,-2,3 that will use all the default colors, windows and reverse-intensities (not turn OFF all reverse intensities).
Example use case: If I import a gel image (where I always want Reverse to be ON), save the reverse intensity, and then view the image in a viewer / app that doesn't yet support Reverse intensity, I want it to default to ON (saved state).

I think I prefer to encode the reverse intensity within the channels string (instead of a separate r=<channel_nameber> parameter) , because it seems more logical to find it there with all the other channel info. Also, all the parsing is done within this c string E.g.

channels, windows, colors, reverses = _split_channel_info(r['c'])
img.setActiveChannels(channels, windows, colors, reverses)

[chris-allan]

Understood, @will-moore.

I assume your ?c=1,-2,3 is referring to ?r=1,-2,3? If not then I'm confused. The use of an additional query string parameter is not mutually exclusive of declaring whether the reverse intensity is enabled or not. Defaulting to the saved state is fine.

The fact of the matter is that the number of codomain maps the rendering engine has is potentially infinite. It is also ordered. Previously this was image wide. Now with #4823 it is per channel. Not taking this into account now and planning for additions sets us up for a situation where we end up breaking the URL style again in a possibly near future release because we didn't take that into account and do it properly to begin with.

I don't think how the view code currently parses the c query string argument is relevant. The important public facing API here is the URL style. If handling codomain maps in a flexible way as far as the URL style is concerned requires twice the view code so be it.

/cc @jburel

[jburel]

Codomain is trickier than color and lut since it is not one or the other:

• order is important i.e. reverse intensity/ then plane slicing != plane slicing then reverse intensity
• the current approach won't scale when we add more codomain. The current model only supports 3 in total but the new model proposed will offer much much more.

It is time to revisit the "practical" approach to something more robust and extensible.
@will-moore let's sit tomorrow or monday to discuss a possible new format by going over the other codomain options.
I still would like to turn them on during 5.3.x

[will-moore]

@chris-allan No, I meant ?c=1,-2,3 E.g. https://nightshade.openmicroscopy.org/webgateway/render_image/3933597/42/0/?c=1,-2,3 uses saved colors etc (and would also saved codomain maps in OMERO 5.3).

To support an extensible list of named Codomain maps per channel, each with a dictionary of parameters and ON/OFF flag, we'd need something like json. E.g. for 2 channels:

maps="1":[{"enabled":true,"name":"reverse"},{"name":"stretch","enabled":true,"yend":250,"ystart":50,"xstart":10,"xend":200}],"2":[{"enabled":false,"name":"reverse"}]

[jburel]

@will-moore: for info, json is the approach taken by the CLI plugin render.py to pass parameters cf. original PR on metadata52 #4542

[jburel]

I will remove the url encoding discussion from this PR since we are still exploring way to encode it
We are getting close to have this PR "mergeable"

Discussed today: we keep the "-r" option for now and open a design issue for the url exploration

[pwalczysko]

Did a quick general Rnd check today, all the bugs seem to be fixed. Behaves as expected.

[pwalczysko]

Created a separate trello card for the SPW grid view thumbs problem. https://trello.com/c/YAeGIgXq/130-thumbnails-rnd-settings-web-plates

[jburel]

URL formatting card: https://trello.com/c/PzMTIpY2/132-rendering-settings-in-url

[jburel]

Could you add tests in test_rendering for codomain support?

[snoopycrimecop]

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

• PR histograms #4703 will-moore 'histograms'
  • components/tools/OmeroWeb/omeroweb/webgateway/static/webgateway/js/omero_image.js

[snoopycrimecop]

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

• PR histograms #4703 will-moore 'histograms'
  • components/tools/OmeroWeb/omeroweb/webgateway/static/webgateway/js/omero_image.js

[snoopycrimecop]

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

• PR histograms #4703 will-moore 'histograms'
  • components/tools/OmeroWeb/omeroweb/webgateway/static/webgateway/js/omero_image.js

[jburel]

https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-Python27/356/ green

[jburel]

Waiting on a final check gh-4850 so we can merge both client PRs.

[jburel]

see https://trello.com/c/h8OylVrf/209-lut-follow-up for slider color when a lut is selected
This will be done in a follow-up PR