Metadata plugin pixelsize #5696

dominikl · 2018-03-21T14:00:13Z

What this PR does

Adds a pixelsize command to the metadata plugin, which allows setting a custom physical pixel size for an Image (or whole Dataset, Project, etc.)

Testing this PR

Set a custom pixel size:
./omero metadata pixelsize --x 5 --y 6 --z 7 --unit nanometer Dataset:123

Doesn't output anything, so use Insight or Web to check if it was successful.

Related reading

IDR/idr0042-nirschl-wsideeplearning#1

/cc @sbesson

jburel · 2018-03-21T14:04:38Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+        for pixel in pixels:
+            if args.x:
+                pixSize = pixel.getPhysicalSizeX()
+                if not pixSize:


to avoid copy/paste of similar code, you should introduce a method to the set the physical size

mtbc · 2018-03-21T14:05:38Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+        elif md.get_type() == "Image":
+            q = """SELECT pix FROM Pixels pix WHERE pix.image.id=:id"""
+        else:
+            raise Exception("Not implemented for type %s" % md.get_type())


This is fine but just to mention that another option is http://downloads.openmicroscopy.org/latest/omero5.4/api/slice2html/omero/cmd/FindChildren.html#FindChildren to find their IDs with typesOfChildren = ['Pixels'] and stopBefore = ['Roi'].

Do we have a python example somewhere which uses FindChildren? Can't get my head around how to use that.

from omero.callbacks import CmdCallbackI from omero.cmd import FindChildren from omero.gateway import BlitzGateway conn = BlitzGateway(stuff) # I suggest user-2 on eel conn.connect() req = FindChildren(targetObjects={'Project': [1]}, typesOfChildren=['Pixels'], stopBefore=['Roi']) prx = conn.c.sf.submit(req) print CmdCallbackI(conn.c, prx).loop(500, 500) conn._closeSession()

and see many IDs of Pixels that are in Project 1.

Thanks! That works. But I think I leave the the hql query for now, because it loads the Pixel objects directly. Otherwise I'd have to get the ids first with FindChildren and then load the Pixel objects from the id list.

Btw, that example would be nice to have on https://docs.openmicroscopy.org/omero/5.4.4/developers/Python.html :-)

Leaving someone else to record or migrate the Python example.

https://trello.com/c/8oJXeUPY/625-findparents-findchildren-example

mtbc · 2018-03-21T14:08:13Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+                if not pixSize:
+                    pixSize = omero.model.LengthI(args.z, units[args.unit])
+                    pixel.setPhysicalSizeZ(pixSize)
+                else:


Are these else worthwhile or simpler just to always do the first clause? (Can instantiate the LengthI outside the loop so the pixels share it?)

Always create a new LengthI object? Is the old one automatically removed from the database then?

Yes, definitely. They are structs.

They don't exist in the database as first-class objects.

mtbc · 2018-03-21T14:09:19Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+                    pixSize.setValue(args.z)
+                    pixSize.setUnit(units[args.unit])
+
+            client.getSession().getUpdateService().saveAndReturnObject(pixel)


Could instead save many in bulk with saveArray outside the loop.

mtbc · 2018-03-22T09:24:18Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+
+        units = {
+            'nm': UnitsLength.NANOMETER,
+            'um': UnitsLength.MICROMETER,


Ought we also allow 'μm'?

mtbc · 2018-03-22T09:25:58Z

Not sure _setPixelsize buys us much but either way certainly looking better! 👍

joshmoore · 2018-03-22T12:23:33Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+        md = self._load(args)
+        client, conn = self._clientconn(args)
+
+        units = {


This lookup can be achieved via:

In [34]: omero.model.LengthI.SYMBOLS.keys()[omero.model.LengthI.SYMBOLS.values().index("mm")] Out[34]: 'MILLIMETER'

so that no hard-coding is needed. You'd have to special case um, though, regardless and unicode is going to be challenging from the terminal.

joshmoore · 2018-03-22T12:24:25Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+        pixelsize.add_argument(
+            "--z", type=float, default=None, help="Physical pixel size Z")
+        pixelsize.add_argument(
+            "--unit", default="um", help="Unit (e.g. nm, um, mm, ...) "


If the sizes need different units, the suggestion would be to call the plugin multiple times?

joshmoore · 2018-03-22T12:24:41Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

+    def pixelsize(self, args):
+        "Set physical pixel size"
+        md = self._load(args)
+        client, conn = self._clientconn(args)


Might be worth doing this after validating the inputs.

joshmoore

A few minor comments added. In general, working with units and the related enums is tricky at best. Just trying to write a snippet for this PR, I already noticed 2-3 helper methods that should be created. But that's for down the road.

Long-term I think we should support 1 or 2 forms of all enum values for a given unit:

MILLIMETER
NANOMETER
etc

and/or

mm
nm
etc.

As long as we're happy to formalize that later and continue to support any extra helpers introduced in this PR (like um) then no objections to not having complete support here.

If there are any concerns over the "one-unit-fits-all" issue, a next step after this PR might be to use the (yet another) helper suggested in https://trello.com/c/Z2NAGjBQ/379-cli-support-enums-for-objpy such that:

 --x=1mm

would be a valid argument.

dominikl · 2018-03-22T16:52:02Z

I'd prefer only using the enum names like --x=1MILLIMETER (guess we could ignore the case, e. g. ~ --x=1millimeter ). It's horrible to type and look at, but at least avoids any unicode issues. But especially then one should have at least the option to specify the unit only once and use it for x, y and z; which is the usual case anyway I'd assume.

dominikl · 2018-03-23T11:18:48Z

I'll leave x, y, z parameter as value only parameter for now. A unit can be specified with --unit parameter and is applied to x, y and z, like before, but I removed the custom unit parsing and use the enum names directly instead ("nanometer", "micrometer", ...). It's just too much hassle to parse stuff like --x=1mm or --x=1inch or could also be --x=1.0ATTOMETER , etc. and can be added later, without breaking current behaviour.

joshmoore · 2018-03-26T19:19:41Z

This has the same issue as addressed in ome/omero-cli-render#6 in that a non-owner thinks that the change has taken place but silently fails:

[jamoore@ome-c6100-1 dist]$ bin/omero hql -q --all "select i from Pixels i order by i.id desc" --limit=2
 # | Class   | Id    | sizeT | dimensionOrder    | sha1                                     | physicalSizeY             | physicalSizeX             | image        | physicalSizeZ            | sizeX | sizeY | sizeZ | details         | sizeC | pixelsType    | significantBits
---+---------+-------+-------+-------------------+------------------------------------------+---------------------------+---------------------------+--------------+--------------------------+-------+-------+-------+-----------------+-------+---------------+-----------------
 0 | PixelsI | 96151 | 200   | DimensionOrderI:1 | fd52d62a98eef4c2d525f176c18afdbb83ad8c47 | 0.112484999001 MICROMETER | 0.112484999001 MICROMETER | ImageI:98401 | 0.20000000298 MICROMETER | 256   | 256   | 1     | owner=5;group=8 | 2     | PixelsTypeI:6 | 16
...
[jamoore@ome-c6100-1 dist]$ OMERO_DEV_PLUGINS=1 bin/omero metadata pixelsize --unit micrometer --x 0.11 --y 0.11 --z 0.3 Image:98401
[jamoore@ome-c6100-1 dist]$

dominikl · 2018-03-27T07:58:31Z

That means I'd need to set the context explicitly {'omero.group': '-1'} ?

joshmoore · 2018-03-27T07:58:48Z

Exactly.

joshmoore · 2018-03-28T10:56:39Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

        params = omero.sys.ParametersI()
        params.addId(md.get_id())
-        pixels = client.getSession().getQueryService()\
+        pixels = client.getSession().getQueryService(ctx)\
                                    .findAllByQuery(q, params)


I think the ctx needs to be passed to the findAllByQuery. On eel I'm getting the same behavior of no change. Likely if not pixels: should also call self.ctx.die or at least self.ctx.err.

Argh, of course, thanks @joshmoore .

joshmoore · 2018-03-28T10:56:52Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

                                    .findAllByQuery(q, params)

        for pixel in pixels:
            if args.x:
-                pixel.setPhysicalSizeX(omero.model.LengthI(args.x, unit))
+                pixel.setPhysicalSizeX(omero.model.LengthI(args.x, unit), ctx)


These don't hurt, but also have no effect.

joshmoore · 2018-03-29T15:27:59Z

Doh. Unfortunately, this leads to:

    serverExceptionClass = ome.conditions.SecurityViolation
    message = Cannot read ome.model.core.Pixels:Id_96252

i.e. ctx needed on saveCollection. All I'm doing is:

login as root
bin/omero hql -q --all "select i from Pixels i order by i.id desc" --limit=1 (i.e. choose the newest image)
OMERO_DEV_PLUGINS=1 bin/omero metadata pixelsize --unit micrometer --x 0.11 --y 0.11 --z 0.3 Image:98502

dominikl · 2018-03-30T12:48:32Z

Bit lost now... When I pass the ctx to the save method I get:

No valid permissions available! DUMMY permissions are not intended for copying. Make sure that you have not passed omero.group=-1 for a save without context

What is that supposed to mean?

joshmoore · 2018-03-30T13:10:17Z

The server is failing to figure how what group you intended to save to, perhaps as a result of saveCollection which could potentially be saving from multiple groups in one go.

mtbc · 2018-04-03T09:03:27Z

Ha, yes, OMERO_DEV_PLUGINS=1 does seem to help with the PR review. 😃

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

https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-python/lastCompletedBuild/testReport/OmeroPy.test.integration.clitest.test_metadata/TestMetadataControl/test_pixelsize/ has been passing.

jburel · 2018-04-05T10:50:37Z

components/tools/OmeroPy/src/omero/plugins/metadata.py

@@ -622,7 +621,9 @@ def pixelsize(self, args):
            if args.z:
                pixel.setPhysicalSizeZ(omero.model.LengthI(args.z, unit))

-        client.getSession().getUpdateService(ctx).saveCollection(pixels)
+        groupId = pixels[0].getDetails().getGroup().getId().getValue()
+        ctx = {'omero.group': str(groupId)}


conn.SERVICE_OPTS.setOmeroGroup(str(groupId)) is another option

For some reason that doesn't work. If I set conn.SERVICE_OPTS.setOmeroGroup(str(groupId)) instead of using the ctx, I'll get a ome.conditions.SecurityViolation when running the command as non-owner of the image.
Same further up: pixels = conn.getQueryService().findAllByQuery(q, params, ctx) works, but when setting conn.SERVICE_OPTS instead of using the ctx, it doesn't.

joshmoore · 2018-04-19T09:40:34Z

Merging for 5.4.6-rc2. Considering this is still a DEV plugin, we might consider a round of refactoring before going full production.

Add pixelsize command to metadata plugin

e734340

jburel reviewed Mar 21, 2018

View reviewed changes

mtbc reviewed Mar 21, 2018

View reviewed changes

Removed duplicated code

4e4522b

mtbc reviewed Mar 22, 2018

View reviewed changes

joshmoore reviewed Mar 22, 2018

View reviewed changes

Use enum names directly to specify unit

39407c2

dominikl force-pushed the metadata_pixelsize branch from 53df243 to 39407c2 Compare March 23, 2018 11:13

dominikl added 2 commits March 27, 2018 10:26

Use group ctx -1

d3ce5bc

Add int. test for metadata pixelsize command

62a7869

dominikl mentioned this pull request Mar 28, 2018

Metadata plugin pixelsize (metadata53) #5703

Closed

joshmoore reviewed Mar 28, 2018

View reviewed changes

Use ctx on findAllQuery

56434af

Use the groupId of the image/dataset/etc.

0dede27

jburel reviewed Apr 5, 2018

View reviewed changes

joshmoore merged commit e34819b into ome:develop Apr 19, 2018

joshmoore added this to the 5.4.6 milestone Apr 19, 2018

Metadata plugin pixelsize #5696

Metadata plugin pixelsize #5696

Conversation

dominikl commented Mar 21, 2018 • edited Loading

What this PR does

Testing this PR

Related reading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikl Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

mtbc Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

dominikl Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore Apr 19, 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 Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbc commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore left a comment

Choose a reason for hiding this comment

dominikl commented Mar 22, 2018

dominikl commented Mar 23, 2018 • edited Loading

joshmoore commented Mar 26, 2018

dominikl commented Mar 27, 2018

joshmoore commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore commented Mar 29, 2018

dominikl commented Mar 30, 2018

joshmoore commented Mar 30, 2018

mtbc commented Apr 3, 2018

mtbc commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore commented Apr 19, 2018

dominikl commented Mar 21, 2018 •

edited

Loading

dominikl Mar 21, 2018 •

edited

Loading

mtbc Mar 21, 2018 •

edited

Loading

dominikl Mar 21, 2018 •

edited

Loading

joshmoore Apr 19, 2018 •

edited

Loading

mtbc Mar 21, 2018 •

edited

Loading

dominikl commented Mar 23, 2018 •

edited

Loading