Wrapper improvements for ROIs

[Tom-TBT]

Changes in this PR are to increase the support of functionalities around ROIs:

• support annotations on ROIs (could be used to organize ROIs with tags, add properties with KV pairs, ...)
• getShapes for an ROI now retrieves wrapped shapes instead of ShapeI

[will-moore]

It is a nice improvement for roi.getShapes() to return ShapeWrapper instead of ShapeI but it is unfortunately a breaking API change.
Although roi.getShapes() isn't really documented or used in our examples, it is currently mapped to roi._obj._getShapes() and it's possible that users have code that would break with that change.

I noticed that _RoiWrapper has

    # TODO: test listChildren() to use ShapeWrapper? or remove?
    CHILD_WRAPPER_CLASS = 'ShapeWrapper'

And roi.listChildren() fails, since this expects a link class such as ProjectDatasetLink etc.
So you could rename your getShapes() to listChildren() and remove the CHILD_WRAPPER_CLASS = 'ShapeWrapper'.

To be consistent with other listChildren() behaviour, IF the shapes aren't loaded then they should be loaded on the fly (and probably cached) as we do in some other places.

[will-moore]

Could you also add support for loading Shapes on the fly in listChildren() if they're not already loaded?

[Tom-TBT]

Hey Will, sorry I'm taking this work back after a long pause. Working back on the tags & ROIs, I would need this to handle tags on ROIs.

Could you also add support for loading Shapes on the fly in listChildren() if they're not already loaded?

I reimplemented _listChildren and listChildren of RoiWrapper, so I don't need to hijack CHILD_WRAPPER_CLASS = 'ShapeWrapper', and so getChildLinks fails as it should.

These should now work:

roi_o = conn.getObject("Roi", 123)
shapes = list(roi_o.listChildren())
annotations = list(roi_o.listAnnotations())

print(shapes)
>> [<_ShapeWrapper id=1714>, <_ShapeWrapper id=1715>]
print(annotations)
>> [<TagAnnotationWrapper id=29>, <TagAnnotationWrapper id=28>]

[will-moore]

Was this change needed to fix something? I don't see that getAnnotationLinkTableName() is used anywhere now.

I've tried to make obj.listAnnotations() work for all object types over at #489, but I'd actually missed that getAnnotationLinkTableName() existed.

[will-moore]

Actually @Tom-TBT, could you remove the changes here and above for getAnnotationLinkTableName() as I've just updated that method to delegate to ann_link_name() in #489 - to cover all object types and this will now conflict with your changes.
Thanks

[Tom-TBT]

Thank you, I removed the changes. It works without it indeed.

[will-moore]

Thanks for getting back to this.
Not tested yet but I'm sure it's working...

I wonder if you could open a test PR to add a test like testGetROICount() at https://github.com/ome/openmicroscopy/blob/a49489e0cb2c30833afa413ae796f65478f6a387/components/tools/OmeroPy/test/integration/test_rois.py#L39 that does a similar setup etc then tests:

roi_o = conn.getObject("Roi", 123)
shapes = list(roi_o.listChildren())

Don't worry about testing roi annotations - I've got that covered in ome/openmicroscopy#6458 for #489

Thanks

[Tom-TBT]

I reverted my changes in getAnnotationLinkTableName. This was supposed to enable listing of Annotation from Roi, but is not needed anymore as #489 fixes it.

I also want to point out another change I have, to list Rois from an Annotation, in getParentLinks
Is it something that should stay here? Do you want to also have something here to not have to hardcode the list of class that can be annotated?

[sbesson]

As we just merge #489 and will be planning an upcoming minor release of OMERO.py, is this an effort we would like to include?

Re-reading the change, I am not particularly enthusiastic about overriding and specializing listChildren() in RoiWrapper. As mentioned in #459 (comment), this API is really targeted for objects which can be linked via an <Object1><Object2>Link object. The roi.shapes relationship uses a different mechanism and is analogous to image.pixels or image.rois relationship where we have explicit ImageWrapper.getROIs() and ImageWrapper.getPrimaryPixels() APIs. In that sense, the original RoiWrapper.getShapes() proposal was the most consistent with the current API.

[will-moore]

@sbesson agreed that is nice if roi.getShapes() returns wrapped shapes, but currently it returns omero.model shapes - see above

#459 (comment)

So, it would be a breaking API change, which I guess is OK if we manage it right?

Well.listChildren() lists the directly-linked Well-Samples, so that is equivalent of roi.shapes

[sbesson]

@sbesson agreed that is nice if roi.getShapes() returns wrapped shapes, but currently it returns omero.model shapes - see above

I was initially confused as I could not see any implementation for getShapes in the source code. Now I see this is happening dynamically in the BlitzObjectWrapper.__getattr__ function.

To avoid backwards-incompatible changes, one option would be to introduce a flag e.g. RoiWrapper.getShapes(wrap=False) and aim to make wrap=True the default behavior in OMERO.py 6.

[Tom-TBT]

Happy to change again the code to go back to getShapes, with the proposed backward compatibility using wrap=False by default.

But should I do it on a new PR? The commit history is getting weird here.

[will-moore]

I still feel that roi.listChildren() is a natural behaviour and consistent with other BlitzGateway wrappers such as well.listChildren(), dataset.listChildren(), image.listAnnotations() etc.

That's a lot easier to remember than roi.getShapes(wrap=True).

Then we get to OMERO.py 6, you can just make roi.getShapes() return the same as roi.listChildren().

We should probably do image.listRois() too!

[sbesson]

To facilitate the discussion, should we collect a reference table of the existing list* and some of the get* APIS for the primary Blitz wrappers and their return types?

[will-moore]

wrapper.listChildren() uses wrapper._listChildren() which uses the self.LINK_CLASS to load children

omero-py/src/omero/gateway/__init__.py

Line 727 in 8990768

def _listChildren(self, ns=None, val=None, params=None):

Wow - I realise there that wrapper.listChildren() is loading all child objects and their annotations!!!
This seems like a big performance loss, since most of the time we never use annotations!

The objects that have a LINK_CLASS are:

$ grep "LINK_CLASS =" src/omero/gateway/__init__.py
    LINK_CLASS = None                                               # BlitzOjectWrapper
    LINK_CLASS = "GroupExperimenterMap"          # ExperimenterWrapper - doesn't work
    LINK_CLASS = "GroupExperimenterMap"          # ExperimenterGroupWrapper - doesn't work!
    LINK_CLASS = "DatasetImageLink"          # DatasetWrapper
    LINK_CLASS = "ProjectDatasetLink"           # ProjectWrapper
    LINK_CLASS = "ScreenPlateLink"          # ScreenWrapper
    LINK_CLASS = None                                 #  PlateWrapper
    LINK_CLASS = None                                 #  WellWrapper
    LINK_CLASS = 'WellSample'                                  #  WellSampleWrapper  not used - no sense!
    LINK_CLASS = None                                  #  ImageWrapper

We also have custom wrapper._listChildren() for Plates and Wells.

So, listChildren() works on:

• Projects

• Datasets

• Screens

• Plates

• Wells

• ExperimenterGroup.listChildren() fails (could not resolve property: name of: ome.model.meta.Experimenter)

• ExperimenterGroup.listParents() returns an empty list

• ExperimenterWrapper.listChildren() fails (ExperimenterWrapper has no child wrapper)

• ExperimenterWrapper.listParents() works! -> returns Groups!

Looking for other list* methods:

$ grep "def list" src/omero/gateway/__init__.py
    def listChildren(self, ns=None, val=None, params=None):
    def listParents(self, withlinks=False):
    def listAnnotations(self, ns=None):
    def listOrphanedAnnotations(self, eid=None, ns=None, anntype=None,
    def listProjects(self, eid=None):
    def listScreens(self, eid=None):
    def listOrphans(self, obj_type, eid=None, params=None, loadPixels=False):
    def listGroups(self):
    def listColleagues(self):
    def listStaffs(self):
    def listOwnedGroups(self):
    def listFileAnnotations(self, eid=None, toInclude=[], toExclude=[]):
    def listOrphanedAnnotations(self, parent_type, parent_ids, eid=None,
    def listTagsInTagset(self):
    def listParents(self, withlinks=True):
    def listPlateAcquisitions(self):
    def listParents(self, withlinks=False):
    def listParents(self, withlinks=False):
    def listParents(self, withlinks=False):
    def listFiles(self):

There's too many get* methods to list here:

$ grep "def get" src/omero/gateway/__init__.py | wc
     220     541    7415

Most of them are to get single items. There are quite a few that return a list - Just looking by eye...:

getParentLinks()
getChildLinks()
getScreens()
getChannelLabels()
getAdministrators()
getCurrentAdminPrivileges()
getAdminPrivileges()
getGroupsLeaderOf()
getGroupsMemberOf()
getObjects()
getAnnotationLinks()
getObjectsByAnnotations()
getObjectsByMapAnnotations()
getEnumerationEntries()
getOriginalEnumerations()
getEnumerations()
getFileInChunks()
getPlanes()
getTiles()
getProjections()
getRenderingModels()
getArchivedFiles()
getImportedImageFiles()
getImportedImageFilePaths()
getDetectors()
getObjectives()
getFilters()
getDichroics()
getFilterSets()
getOTFs()
getLightSources()

Summary

wrapper.listChildren() works on 5 wrappers - 3 of which have LINK_CLASS and 2 have custom _listChildren()
These are all for traversing a clear parent-child hierarchy.

Proposing to add roi.listChildren() would mean we have 3 wrappers with custom _listChildren().
This is also a clear parent-child hiearchy.

We have lots of get*() methods, so roi.getShapes() would also be perfectly valid.

NB: it's also clear there's a bunch of stuff in the BlitzGateway that could be cleaned up.
I don't like wrapper.listChildren() including child annotations by default.
Also group/experimenter listParents/listChildren should be removed or fixed.

[sbesson]

Thanks for the summary, there is a lot of history here.

My biggest reservation with BlitzObjectWrapper.listChildren is the ambiguous nature of its contract. In practice, it's only functional for the project/dataset/image or plate/well/wellsample/image hierarchies currently. Given the multitude of relationships for every object, if we decide to override listChildren to RoiWrapper, we should clearly defined what is considered as a child in

omero-py/src/omero/gateway/__init__.py

Lines 757 to 763 in 8990768

	def listChildren(self, ns=None, val=None, params=None):
	"""
	Lists available child objects.
	:rtype: generator of :class:`BlitzObjectWrapper` objs
	:return: child objects.
	"""

.

Alternatively, we could introduce RoiWrapper.listShapes to return wrapped shapes and mirror other named APIs like listAnnotations, listPlateAcquisitions.

[will-moore]

Thanks @sbesson.
For me, the ROI->Shape relationship is a clear Parent-Child relationship, so roi.listChildren() wouldn't be confused with listing other ROI links like annotations or images etc.
So, I still have a preference for listChildren() and updating the BlitzObjectWrapper.listChildren() docs as suggested.

But I would also be OK with your alternative suggestion of roi.listShapes() since that avoids the breaking API change of getShapes().