Annotation count web by dominikl · Pull Request #5023 · ome/openmicroscopy

dominikl · 2017-01-11T11:05:39Z

What this PR does

Show the number of annotations available in the right hand side panel.

Testing this PR

Annotate different objects (Images, Datasets, etc.). Select these objects (also some non-annotated objects) and make sure the annotation count shown is correct. Test with single as well as multi (batch) selection.

Related reading

Trello - RFE: Indication of number of annotations on an image in brackets (5) on accordion tabs in right hand pane.

/cc @will-moore

will-moore · 2017-01-11T11:26:52Z

components/tools/OmeroWeb/omeroweb/webclient/controller/container.py

You'll need a 3rd argument here so that the group context is right
findAllByQuery(q, params, self.conn.SERVICE_OPTS)
otherwise I think it will only query your default group.

will-moore · 2017-01-11T11:31:55Z

components/tools/OmeroWeb/omeroweb/webclient/controller/container.py

Minor point: I think you shouldn't need the fetch for parent as you don't need to retrieve parent for anything (just need join for the query itself).

will-moore · 2017-01-11T11:46:19Z

Here you're counting annotation 'links' rather than the annotations themselves. E.g. if 2 users add the same Tag, this will show up as (2) which is different from what Insight is doing.

However, it is consistent with the way webclient displays Tags in the single-object right panel (each tag is shown once for every time it is linked) but not in batch-annotate panel (all links for each tag are collapsed into single tag).
If we want the number to match the number of Tags shown in each panel, then we'd need to count them differently for batch vv single object.
Probably better to always count Annotations rather than links, and we can consider changing the display of Tags for single object later. I know @jburel has been advocating this for some time!

will-moore · 2017-01-11T11:47:12Z

A few flake8 errors to fix for travis.

will-moore · 2017-01-11T11:50:17Z

...eroWeb/omeroweb/webclient/templates/webclient/annotations/includes/metadata_general_tag.html

You don't need this loop, you can simply do {{ annotationCounts.TagAnnotation }}.

atarkowska

Thank for tackling the card. Some comments in the code.

BlitzObjectWrapper offers listAnnotations https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/src/omero/gateway/__init__.py#L949. Why not add countAnnotations there? To me looks like it should to to gateway

atarkowska · 2017-01-11T11:19:13Z

components/tools/OmeroWeb/omeroweb/webclient/controller/container.py

could you please use constant omero.constants.metadata.NSINSIGHTRATING

atarkowska · 2017-01-11T11:19:21Z

components/tools/OmeroWeb/omeroweb/webclient/controller/container.py

could you please use constant omero.constants.metadata.NSINSIGHTRATING

atarkowska · 2017-01-11T11:25:04Z

components/tools/OmeroWeb/omeroweb/webclient/controller/container.py

please do not use object as a var, just obj

atarkowska · 2017-01-11T11:39:18Z

...eb/omeroweb/webclient/templates/webclient/annotations/includes/metadata_general_comment.html

no need to loop. just({{ annotationCounts.CommentAnnotation }})

atarkowska · 2017-01-11T11:39:33Z

...roWeb/omeroweb/webclient/templates/webclient/annotations/includes/metadata_general_file.html

no need to loop. ({{ annotationCounts.FileAnnotation }})

atarkowska · 2017-01-11T11:39:44Z

...eroWeb/omeroweb/webclient/templates/webclient/annotations/includes/metadata_general_map.html

no need to loop. ({{ annotationCounts.MapAnnotation }})

atarkowska · 2017-01-11T11:39:52Z

...oWeb/omeroweb/webclient/templates/webclient/annotations/includes/metadata_general_other.html

no need to loop. ({{ annotationCounts.OtherAnnotation }})

atarkowska · 2017-01-11T11:40:05Z

...Web/omeroweb/webclient/templates/webclient/annotations/includes/metadata_general_rating.html

no need to loop. ({{ annotationCounts.LongAnnotation }})

atarkowska · 2017-01-11T11:40:16Z

...eroWeb/omeroweb/webclient/templates/webclient/annotations/includes/metadata_general_tag.html

no need to loop. ({{ annotationCounts.TagAnnotation }})

atarkowska · 2017-01-11T11:45:27Z

components/tools/OmeroWeb/omeroweb/webclient/controller/container.py

we are repeating the same code, could you move them to helpers please?

dominikl · 2017-01-12T11:17:01Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

I should move these methods somewhere else. Where's a good place for utilility/helper methods @aleksandra-tarkowska ?

will-moore · 2017-01-12T14:38:42Z

Web error reported at
/webclient/metadata_details/acquisition/160/

 serverStackTrace = ome.services.query.QueryException: Illegal query
None is not mapped [
           select al from None al
           left outer join al.parent as pa
           left outer join fetch al.child as an
           where pa.id = :id
           ]

will-moore · 2017-01-12T15:45:48Z

That error was also causing 'Run' right panel to fail in a bunch of robot tests https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-robotframework/497/robot/

atarkowska · 2017-01-12T15:52:26Z

could you please add gateway tests?

atarkowska

thanks for the changes. still few suggestions

atarkowska · 2017-01-12T16:05:40Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

This could be named countAnnotations, but up to you. I would also expect ns as a param to match listAnnotations

Couldn't use countAnnotations because it's also defined in https://github.com/dominikl/openmicroscopy/blob/c99b0e5c0e7535326d947cc4f0e04666cf938234/components/tools/OmeroWeb/omeroweb/webclient/webclient_gateway.py#L2040 . I'll rename both methods to getAnnotationCounts to be consistent.

How about moving https://github.com/dominikl/openmicroscopy/blob/c99b0e5c0e7535326d947cc4f0e04666cf938234/components/tools/OmeroWeb/omeroweb/webclient/webclient_gateway.py#L2040 down to gateway then. no need for 2

So I think we do not want to move self.annotation_counter but the method could be unified in the main gateway so we don't need to deprecate anything. Happy to chat

atarkowska · 2017-01-12T16:09:15Z

components/tools/OmeroWeb/omeroweb/webclient/controller/container.py

why not return self._get_object().getAnnotationCount()

atarkowska · 2017-01-12T16:16:39Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

sorry for being picky, why that has to be a list of strings type-id? In the views.batch_annotate you have a dict objs of real objects https://github.com/dominikl/openmicroscopy/blob/c99b0e5c0e7535326d947cc4f0e04666cf938234/components/tools/OmeroWeb/omeroweb/webclient/views.py#L1969.

example https://github.com/dominikl/openmicroscopy/blob/c99b0e5c0e7535326d947cc4f0e04666cf938234/components/tools/OmeroWeb/omeroweb/webclient/controller/container.py#L238

So you could use objtype=o.OMERO_CLASS directly without a need of getAnnotationLinkTableName

atarkowska · 2017-01-12T16:38:26Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

why do you need getAnnotationLinkTableName?
How about

q = """select al from %sAnnotationLinks al left outer join al.parent as pa left outer join fetch al.child as an where pa.id = :id """ % self.OMERO_CLASS

atarkowska · 2017-01-13T10:23:14Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

empty dict in python is declared as mydict = {} see https://docs.python.org/2/library/stdtypes.html

This will throw exception in a loop

atarkowska · 2017-01-13T14:26:05Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

as Annotation are in the same table I would assume they shouldn't have the same id regardless of type, so unique list could be constructed from the annotationlinks.

Then you could just do: uniqueIds = set([al._child._id._val for al in annotationlinks])

if you need per type maybe just use dict {'type': [ id, ...]} rather then parse string

just a suggestion

atarkowska · 2017-01-13T14:35:28Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

interesting, is that passing flake8? I think q = q + " and an.ns in (:ns)"

atarkowska · 2017-01-13T14:44:57Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

how about to replace that long conditional statement with obj_type.title().replace("Plateacquisition", "PlateAcquisition")

atarkowska · 2017-01-16T18:20:12Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

is that really an optional arg?

atarkowska · 2017-01-16T18:20:33Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

if you declare as a list you don't need that bit

atarkowska · 2017-01-16T18:22:18Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

isinstance please

How shall I use isinstance to check if the type of the annotation is part of the dictionary atypes?

isinstance(al._child, tuple(atypes.keys())) https://docs.python.org/2/library/functions.html#isinstance

Ideally if you could load atypes from the registry of abstract class. https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/src/omero/gateway/__init__.py#L4599
Maybe look at this later as an improvement step

atarkowska · 2017-01-16T18:29:12Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

this is inconvenient, accordion is now configurable https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroWeb/omeroweb/settings.py#L663, would you like to take it to the account? How about other way around, building dict of types from annotationlinks

atarkowska · 2017-01-16T18:31:07Z

I am guessing https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroWeb/omeroweb/settings.py#L663 could be taken to the account in HQL query as well

will-moore · 2017-02-28T14:58:31Z

components/tools/OmeroPy/src/omero/gateway/__init__.py


        ctx = self.SERVICE_OPTS.copy()
-        ctx.setOmeroGroup(self.group)
+        ctx.setOmeroGroup(None)


I found that when you are querying in a group that is NOT your default group, you actually need to use ctx.setOmeroGroup(-1) here, otherwise you don't see anything:

will-moore · 2017-03-01T09:47:18Z

Working fine across different groups now.
Good to merge.

atarkowska · 2017-03-01T10:08:15Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

+
+    for r in queryResult:
+        ur = unwrap(r)
+        if ur[3] == '/basic/num/long/':


#5056 was merged, is there anything stopping you from using type(an.class)?

jburel · 2017-03-01T10:10:00Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

+        queryResult = self.getQueryService().projection(q, params, ctx)
+
+        for r in queryResult:
+            ur = unwrap(r)


same code as above (line 190). A helper method will be better

I think @dominikl just forgot to remove the old method when moving it into the Gateway in b39c5f6

Thanks. Yes, forgot to remove the old code (line 190+).

dominikl · 2017-03-01T13:12:52Z

--depends-on #5139

jburel · 2017-03-02T06:28:17Z

Thanks for removing the code.
When I add a tag for example, the tag is added but I was expected to be reflected in the count.
This is not the case until I reload the right hand panel
This could be an RFE done in another PR (not 5.3.0) since this PR is getting long

dominikl · 2017-03-02T08:01:12Z

Yes, @will-moore suggested a follow-up PR for that problem, too #5023 (comment)

jburel · 2017-03-02T09:10:24Z

@dominikl so I missed it in the long thread. I will add a card

jburel · 2017-03-02T09:33:33Z

card https://trello.com/c/2YHUymUw/18-rfe-increment-count

will-moore reviewed Jan 11, 2017

View reviewed changes

atarkowska suggested changes Jan 11, 2017

View reviewed changes

jburel added the develop label Jan 12, 2017

dominikl commented Jan 12, 2017

View reviewed changes

atarkowska reviewed Jan 12, 2017

View reviewed changes

atarkowska reviewed Jan 13, 2017

View reviewed changes

dominikl closed this Jan 13, 2017

dominikl reopened this Jan 13, 2017

dominikl closed this Jan 13, 2017

dominikl reopened this Jan 13, 2017

dominikl closed this Jan 16, 2017

dominikl reopened this Jan 16, 2017

dominikl closed this Jan 16, 2017

dominikl reopened this Jan 16, 2017

atarkowska reviewed Jan 16, 2017

View reviewed changes

components/tools/OmeroPy/src/omero/gateway/__init__.py Outdated

Copy link
Copy Markdown

Member

atarkowska Jan 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that really an optional arg?

atarkowska reviewed Jan 16, 2017

View reviewed changes

components/tools/OmeroPy/src/omero/gateway/__init__.py Outdated

Copy link
Copy Markdown

Member

atarkowska Jan 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you declare as a list you don't need that bit

atarkowska reviewed Jan 16, 2017

View reviewed changes

dominikl added 3 commits February 27, 2017 16:03

Move countAnnotations into Gateway

b39c5f6

Remove int() conversions

28d401b

Explicitly set OmeroGroup ctx to None

2765016

dominikl force-pushed the annotation_count_web branch from f7fe051 to 2765016 Compare February 27, 2017 16:16

dominikl closed this Feb 27, 2017

dominikl reopened this Feb 27, 2017

dominikl closed this Feb 27, 2017

dominikl reopened this Feb 27, 2017

dominikl closed this Feb 27, 2017

dominikl reopened this Feb 27, 2017

will-moore reviewed Feb 28, 2017

View reviewed changes

Set omero group to -1

1511b28

atarkowska reviewed Mar 1, 2017

View reviewed changes

jburel reviewed Mar 1, 2017

View reviewed changes

Use type(an.class); some code cleanup

c3aa424

dominikl closed this Mar 1, 2017

dominikl reopened this Mar 1, 2017

dominikl closed this Mar 1, 2017

dominikl reopened this Mar 1, 2017

dominikl closed this Mar 1, 2017

dominikl reopened this Mar 1, 2017

jburel merged commit 7fd4d89 into ome:develop Mar 2, 2017

jburel added this to the 5.3.0 milestone Mar 29, 2017

dominikl deleted the annotation_count_web branch May 3, 2017 09:21

Conversation

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

Uh oh!

What this PR does

Testing this PR

Related reading

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

will-moore commented Jan 11, 2017

Uh oh!

will-moore commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atarkowska left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

will-moore commented Jan 12, 2017

Uh oh!

will-moore commented Jan 12, 2017

Uh oh!

atarkowska commented Jan 12, 2017

Uh oh!

atarkowska left a comment

Choose a reason for hiding this comment

Uh oh!

atarkowska Jan 12, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atarkowska Jan 12, 2017 • 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!

atarkowska Jan 12, 2017 • 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!

atarkowska Jan 12, 2017 • 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!

atarkowska Jan 13, 2017 • 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!

atarkowska Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

dominikl commented Jan 11, 2017 •

edited

Loading

atarkowska Jan 12, 2017 •

edited

Loading

atarkowska Jan 12, 2017 •

edited

Loading

atarkowska Jan 12, 2017 •

edited

Loading

atarkowska Jan 12, 2017 •

edited

Loading

atarkowska Jan 13, 2017 •

edited

Loading

atarkowska Jan 13, 2017 •

edited

Loading

atarkowska Jan 13, 2017 •

edited

Loading

atarkowska Jan 13, 2017 •

edited

Loading

atarkowska Jan 20, 2017 •

edited

Loading

atarkowska Jan 16, 2017 •

edited

Loading

atarkowska Mar 1, 2017 •

edited

Loading