Some bugfixes by dominikl · Pull Request #15 · ome/omero-cli-render

dominikl · 2018-11-16T14:27:12Z

This PR fixes some bugs I noticed while trying to export ('render info') and apply ('render set') the rendering settings of idr0044:

The channel index was off by 1 (when setting the name and activate/deactivate)
'info' command output used 0-based index, whereas the 'set' command expects 1-based index for the channels
start and end values specified in the yaml were ignored (when setting the rendering settings)
Instead of start/end the min/max values specified in the yaml were set as window start/end.
If a channel is not specified in the yaml it is deactivated by the the set_active_channels method. I added a workround earlier, which is not buggy (afaik) but I changed it a bit, so its clearer and safer.

Test: With this PR it should now be possible to adjust the rendering settings of an image in OMERO.web. Then export them with ./omero render info --style yaml Image:ID > renderingsettings.yml And then reapply them with ./omero render set Image:ID renderingsettings.yml. The renderings settings should then be the same.

This PR adds support for a 'version' field in the yml/json. If rendering settings specify start/end and min/max you have to add the version, otherwise it's not clear which values to use for the channel window start/end. Earlier versions used min/max, but with this version (version 2) start/end will be used.

jburel · 2018-11-16T15:04:35Z

./src/omero_cli_render.py:534:68: W291 trailing whitespace

sbesson · 2018-11-19T16:12:27Z

With the last commit, the test_set_single_channel integration tests are now all failing probably due to the channel indexing changes introduced in this PR

Unrelated to this, the tests are quite slow to run which does not help troubleshooting. I might try to have a go at profiling where most of the time is spent to see if we can speed up things.

sbesson · 2019-01-16T12:14:18Z

src/omero_cli_render.py

        rangelist = []
        colourlist = []
        for (i, c) in newchannels.iteritems():
+            i += 1


I am still unclear why this offset is necessary. Looking at IDR/idr0047-neuert-yeastmrna#11 (comment), could the confusion come from the assumption that the channels loaded from the YAML/JASON files are 0-indexed?

Ah, so that's why the export via 'info' command import via 'set' didn't work. Ok, maybe it's better in that case to not increase by one, but fix the output of the 'info' command.

dominikl · 2019-01-16T15:30:27Z

Removed the +1 offset again. Also integration test needed some adjustment.

General remark: With this PR start and end need to specified for the set command, min and max are ignored (as this is the min/max pixel value, which cannot be set anyway). Have to check for potential consequences for older rendering settings definitions.

dominikl · 2019-01-16T15:35:58Z

src/omero_cli_render.py

            else:
                cindices.append(i)
-            rangelist.append([c.min, c.max])
+            rangelist.append([c.start, c.end])


Addtionally checking here for c.min and c.max here, could make the changes of this PR backward compatible. The question then is, what should be done if min/max and start/end are specified:

min/max overrides start/end

start/end take precedence

throw an error

I think we will have to handle previous versions of the YML files especially since we extensively used this format in IDR.

My understanding is that below are the proposed behaviors for the two format versions:

version 1 (current): channel min/max sets the channel window start/end, channel start/end ignored (or maybe simply not part of the render YML/JSON specification?)

version 2 (current): channelstart/end sets the channel window start/end, min/max is now either a no-op or not present for the YML/JSON specification

One way we might consider to make this unambiguous would be to start introduce a version: 2 field in the YAML/JSON specification. The question now remains: what would happen when such version field is absent? Options are:

always default to version: 1. This means that a YML file with start/end without version: 2 would probably not behave as expeced.

as suggested above, detect and error if start/end and min/max are both found in the format. A potential implication is that a YML file with only start/end and no version should be auto-detected as version 2 while a YML file with only min/max and no version would be autodetected as version 1.

I'd vote for the last option. Add a version field. If it's not specified start/end implies version 2, min/max implies version 1, if both specified error.

dominikl · 2019-01-17T11:28:01Z

Shall I go ahead with option "Add a version field. If it's not specified start/end implies version 2, min/max implies version 1, if both specified error." ?
That would make the script backward compatible with older rendering setting definitions we have in idr. Just noticed for some of the recent idr project I used renderings settings with 0 channel indexing, will have to change these.
If we go ahead with that, I'd also change the 'info' command to add 'version: 2' to the output and also use 1 indexing for channels, so that the round trip render info --style yaml Image:123 > xyz.yml render set Image:123 xyz.yml works.

The 'version' field enables backwards compatibility; 1-indexed output makes a it possible to export via info command and import via set command.

dominikl · 2019-01-18T08:54:21Z

That is weird... if I move the failed test (which can't have anything to do with the PR actually) to the beginning, it passes.

joshmoore · 2019-01-18T12:12:47Z

It's probably not critical yet, but moving forward if we want to do this more often, I'd imagine separating out the logic for each version into a helper will help us from breaking older versions (even at the cost of slightly higher duplication) cF hdfstorageV1 vs hdfstorageV2

sbesson

A few inline comments. Happy to discuss them over voice. Otherwise, 👍 for integrating these changes as the tests are green.

Thinkinof the next steps before omero-cli-render 0.4.0 is released, we will need:

a freeze of the specification for version 2 of the rendering format
a review of the CLI command helps and the info/set round-tripping
possibly a review of the test coverage

sbesson · 2019-01-18T12:30:34Z

src/omero_cli_render.py

 """

+# Current version for specifying rendering settings
+currentSpecVersion = 2


maybe capitalize this e.g. SPEC_VERSION

sbesson · 2019-01-18T12:54:44Z

src/omero_cli_render.py


+        # Previously min/max was used to set the channel window start/end
+        # From version 2 on start/end will be used.
+        if 'version' not in data:


Up for discussion, the alternative to handling the versioning in the command would be to move this logic up to ChannelObject.init_from_dict() i.e. serialize to a unified ChannelObject representation independently of the input version.

This might lead us to considerations along the lines of #15 (comment) i.e. start have versioned loaders.

I can write json-schema if it's helpful ;)

sbesson · 2019-01-23T07:59:45Z

Briefly discussed with @dominikl and @joshmoore. Merging this and tagging as 0.4.0a1 so that we can start consuming v2 and gaining experience on it. There are a few items reported above which should be reviewed before we tag 0.4.0 but this can be addressed as separate more specific PRs.

sbesson · 2019-01-23T08:18:26Z

Pre-release now available as https://pypi.org/project/omero-cli-render/0.4.0a1/

Some bugfixes

951aaba

Flake8

146bee3

sbesson reviewed Jan 16, 2019

View reviewed changes

sbesson mentioned this pull request Jan 16, 2019

render info returns 0-indexed channels #18

Closed

Assume indices start with 1 for 'set' command

fee0e89

dominikl commented Jan 16, 2019

View reviewed changes

sbesson mentioned this pull request Jan 17, 2019

Add support for defaultZ/defaultT #16

Closed

sbesson added this to the 0.4.0 milestone Jan 17, 2019

dominikl added 3 commits January 17, 2019 15:21

Add 'version' field; make info output 1-index based

06b6053

The 'version' field enables backwards compatibility; 1-indexed output makes a it possible to export via info command and import via set command.

Adjust integration tests for the versioning

a70d94e

Swap order of tests

51c8a1c

dominikl mentioned this pull request Jan 18, 2019

Fix channel index offset IDR/idr0044-mcdole-tardislightsheet#6

Merged

sbesson reviewed Jan 18, 2019

View reviewed changes

sbesson merged commit 5595ded into ome:master Jan 23, 2019

dominikl mentioned this pull request Jan 23, 2019

Improve versioning code #20

Merged

sbesson mentioned this pull request Jun 23, 2020

Fix reactivate channels #40

Merged

dominikl mentioned this pull request Jul 28, 2025

Fix save rdef. set rdef.version BioNGFF/omero-import-utils#19

Merged

Conversation

dominikl commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jburel commented Nov 16, 2018

Uh oh!

sbesson commented Nov 19, 2018

Uh oh!

sbesson Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominikl Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

dominikl commented Jan 16, 2019

Uh oh!

dominikl Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

sbesson Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

dominikl Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

dominikl commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dominikl commented Jan 18, 2019

Uh oh!

joshmoore commented Jan 18, 2019

Uh oh!

sbesson left a comment

Choose a reason for hiding this comment

Uh oh!

sbesson Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

sbesson Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

joshmoore Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

sbesson commented Jan 23, 2019

Uh oh!

sbesson commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dominikl commented Nov 16, 2018 •

edited

Loading

sbesson Jan 16, 2019 •

edited

Loading

dominikl commented Jan 17, 2019 •

edited

Loading