-
Notifications
You must be signed in to change notification settings - Fork 35
Add more opts support for getObjects('Annotation') #489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 9 commits
c86f8e2
16dc35a
4d6c32e
559eb61
4c1ee1d
b0e6410
367e22e
97c30cc
9a2ce90
513ce54
2ce949c
d16cce3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5164,13 +5164,55 @@ def _getQueryString(cls, opts=None): | |
| Returns a tuple of (query, clauses, params). | ||
|
|
||
| :param opts: Dictionary of optional parameters. | ||
| NB: No options supported for this class. | ||
| ann_type: (optional) "tag", "file", "comment", "long", "map" | ||
| parent_type: (optional) "project", "dataset", "image" etc | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a list of all objects that can be annotated?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't actually know where to find such a list or if it's possible to create one?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://omero.readthedocs.io/en/stable/developers/Model/EveryObject.html#annotation seems like a good start... |
||
| parent_ids: (optional) list of IDs for the parent type | ||
| ns: (optional) namespace string to filter by | ||
| :return: Tuple of string, list, ParametersI | ||
| """ | ||
|
|
||
| query, clauses, params = super( | ||
| AnnotationWrapper, cls)._getQueryString(opts) | ||
| if opts is None: | ||
| opts = {} | ||
|
|
||
| query = ("select obj from Annotation obj " | ||
| "left outer join fetch obj.file as file " | ||
| "join fetch obj.details.owner as owner " | ||
| "join fetch obj.details.creationEvent") | ||
| return query, [], omero.sys.ParametersI() | ||
|
|
||
| ann_type = opts.get('ann_type', None) | ||
| if ann_type == "tag": | ||
| clauses.append("obj.class=TagAnnotation") | ||
| elif ann_type == "file": | ||
| clauses.append("obj.class=FileAnnotation") | ||
| elif ann_type == "comment": | ||
| clauses.append("obj.class=CommentAnnotation") | ||
| elif ann_type == "long": | ||
| clauses.append("obj.class=LongAnnotation") | ||
| elif ann_type == "map": | ||
| clauses.append("obj.class=MapAnnotation") | ||
| elif ann_type is not None: | ||
| msg = ("ann_type '%s' not recognised. Must be one of 'tag', 'file', " | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be implemented for all supported annotations (BooleanAnnotation, TermAnnotation, XmlAnnotation...) |
||
| "'comment', 'long', 'map'" % ann_type) | ||
| raise AttributeError(msg) | ||
|
|
||
| if 'parent_type' in opts: | ||
| obj_type = opts['parent_type'].title().replace("Plateacquisition", "PlateAcquisition") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using this list (from https://omero.readthedocs.io/en/stable/developers/Model/EveryObject.html#annotation) I see only these cases: If the |
||
| ids_clause = "" | ||
| if 'parent_ids' in opts: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the real-world use case associated with specifying |
||
| ids_clause = f"and link.parent.id in (:parent_ids)" | ||
| params.add("parent_ids", rlist([rlong(i) for i in opts['parent_ids']])) | ||
|
|
||
| clause = f"""exists (from {obj_type}AnnotationLink as link | ||
| where link.child.id = obj.id {ids_clause})""" | ||
| clauses.append(clause) | ||
|
|
||
| if 'ns' in opts: | ||
| clauses.append("obj.ns=:ns") | ||
| params.add("ns", rstring(opts['ns'])) | ||
|
|
||
| return (query, clauses, params) | ||
|
|
||
| @classmethod | ||
| def _register(cls, regklass): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably this could use the full annotation type
TagAnnotation,FileAnnotationand save the mapping below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean
"TagAnnotation"(string) oromero.model.TagAnnotation(class)?I feel that it's more user-friendly for the
BlitzGatewaymethods to take strings rather thanomero.modelobjects. E.g.conn.getObject("Image")rather thanconn.getObject(omero.model.ImageI).So I'd like to do the mapping here. Otherwise we'd have to do the mapping in
omero-webfor ome/omero-web#682 and probably other places too.I have just realised that the more natural way to do this (since we only support a single ann type) is to do:
e.g. get Tags on datasets... NB: we already support
conn.getObject("TagAnnotation")instead of
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that's a more natural extension of the existing API and substantially simplifies the logic.
I think it would make sense to use the same semantics for the
parent_typee.g. when retrieving all the tags under a tagset, one could useThis would remove the special casing discussed in #489 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually,
conn.getObjects("dataset')orconn.getObjects("tagannotation")is already case-insensitive, so lets stick with that here too for consistency.