ome / omero-web

Django-based OMERO.web client
https://www.openmicroscopy.org/omero
16 stars 29 forks source link

Inherit annotation support #508

Closed Tom-TBT closed 7 months ago

Tom-TBT commented 8 months ago

This PR is to discuss the implementation of support for inherited annotations.

On the webclient api front, this PR include changes to api_annotations, which now also includes a field on inherited annotations in the returned JSON:

{
  "annotations": [],    // list of annotations (no change)
  "experimenters": [],  // list of experimenters (no change)
  "inherited": { // new
      "annotations": [], // list of inherited annotations (no duplicates)
      "inheritors": {}   // annID dictionnary of inheritors. Same style as annotations.link.parent
}

To use it, you would have to "unpack" the inherited annotations to populate a list of annotation, one for each matching inheritor. The annotations are then processed normally.

For example, I used this changes to add the results in the main UI of the Key-Value pairs, and look like that (from a dataset object): image

See other example with parallel PR on support with OMERO.figure

I had to implement a recursive function to list parents of objects, because obj.getAncestry() does not go through all links if an image is inside multiple datasets. Was there a better way to do that ?

I hope that it's not too hacky in the way it's implemented. Looking forward from comment / feedback

will-moore commented 8 months ago

Thanks for the PR @Tom-TBT. I gave it a quick test locally and seems to be working well. It's definitely useful functionality. I have a couple of broad concerns:

Screenshot 2023-11-02 at 12 58 01

However, I think the summary needs some work: In that view 5 Annotations linked to: 5 objects is not quite true since it's one KVP on the one Dataset. Maybe Dataset:55:CDK5RAP2-C linked to 5 objects. And the same is true in the tooltip:

Screenshot 2023-11-02 at 12 58 57

I was trying to think of a way that the JSON format could be more generic (less specific to the particular UI that you want to show here), but it's hard to do that without adding a lot more data to the response (e.g. all P/D/I links) or requiring the client to make multiple calls. E.g lookup the P/D/I links and then make a call to include parent KVP e.g.

webclient/api/annotations/?type=map&image=10&image=11&dataset=2&project=3

This wouldn't need any api changes.

Tom-TBT commented 8 months ago

How to handle the UI space

I thought it worked but apparently not: It was supposed to list the KV from the topmost to the lowest:

In any case, we could leave the current object annotation on top, and then list the inherited below in reverse order (to avoid having the order change depending on which object in the hierarchy is selected).

it's possible that not all the selected objects are inherited from the same parents

And that's the case too for OMERO.figure, where images can come from wells & datasets

Dataset:55:CDK5RAP2-C linked to 5 objects

Maybe then gets crowded with the name. Should we drop the name? And with the tooltip, that's some more UI changes required in the HTML template then!

webclient/api/annotations/?type=map&image=10&image=11&dataset=2&project=3

Then we need to know first the hierarchy of each object queried, and tie back all the parent annotations in the response to the children.

Are you able to get the full hierarchy without changing the API?

I saw the api_parent_links : webclient/api/parent_links/?image=11 It supports only parent_types = {"image": "dataset", "dataset": "project", "plate": "screen"} right now (so wouldn't work for plates), but, adding the implementation for {"image": ["dataset", "well"], "well": "acquisition", "acquisition": "plate"} sounds better than the questionable changes I made in api_annotation

Thanks for the feedback!

joshmoore commented 8 months ago

There's not yet anything to identify whether or not a parent Annotation should apply to a child, right? I.e. all are inherited.

Tom-TBT commented 8 months ago

@joshmoore I don't know if anything exists to say that. Though objects have the listParent and listChildren methods ;)

But to add to your comment, I could think of two flags:

will-moore commented 8 months ago

Looking better:

Screenshot 2023-11-09 at 15 41 16

will-moore commented 8 months ago

I think this is working pretty well now...

Screenshot 2023-11-14 at 12 55 18

Maybe some tweaks to the API:

So I'm pretty happy with this PR. Haven't tested Screen/Plate/Well yet. But I'd be interested in hearing from @knabar

Tom-TBT commented 8 months ago

Thanks Will for the feedback, I'm making those three changes now.

On the python side I made tests with a different approach to get the hierarchy. So far I had this recursive function, but the objects are processed one by one. Replacing it with HQL queries and it goes much faster. The code is longer but I think it's worth.

Here's a python file if you want to make the tests for comparison: benchmark.zip And here are some results from my test server image

So I should definitely use HQL to get the parents of each object.

Either way, I'm building a dictionary where parent types are the key. Assuming I query the image with ID 123 & 456, I'm buidling a dictionary that looks like this:

{
  "ProjectI": {
    "1" : [ # The keys are the parent object IDs (project ID here)
      {"id": "123", "class": "ImageI", "name": "img_name" },
      {"id": "456", "class": "ImageI", "name": "img_name_2" },
    ]
  }
  "DatasetI": {
    "23" : [
      {"id": "123", "class": "ImageI", "name": "img_name" },
      {"id": "456", "class": "ImageI", "name": "img_name_2" },
    ],
    "45" : [
      {"id": "123", "class": "ImageI", "name": "img_name" },
    ]
  }
}

Can you help me figure where the function returning such dictionary belongs? Should I have this function just for views.api_annotations , or should I move it to tree.py as a "marshal function" ?

knabar commented 8 months ago

Looking good to me, I just have a question regarding users who may have lots of annotations, but no use case for seeing these extra annotations in the UI.

will-moore commented 8 months ago

@Tom-TBT I imported a lot of fake files into the merge-ci server so you can test performance. You can see (and select) 500 of them by using search. E.g. /webclient/search/?search_query=fake. They are currently all in one Dataset and don't have KVP - let me know if you want me to run some script or annotate them in some way?

Tom-TBT commented 8 months ago

I made some test on the time it takes for the annotation query. I did it with:

image

The time increases when I increase the number of annotations, not when I increase the number of dataset or project.

The increase of the response time increase linearly with the number of image selected, which is also expected.

Regarding the display in the UI, I'm able to select up to ~330 images (after that, the query is too long, could be compressed to image=1,2,3). With that many images and all the annotations I have from the previous test (with even extra KV on the other 230 images), the page seems ok: image

Tom-TBT commented 7 months ago

I found an issue with HCS data. When annotations on the Well or on the PlateAcquisition, the display in the webclient of KV fails. This is because there are no name for Well or Run (name is optional for Run) while it should print the name of the parent object (Added on Project XYZ)

A quickfix is to set the link.parent.name to "", but it would be better to show the "name" of the well, or to build the run name ("Run "+ run_id). I need a little bit more time to figure where to do it, and without adding too much extra loading just for that feature (Well coordinates depends on the plate object).

Tom-TBT commented 7 months ago

Last commit fixes the name issue of the Well and PlateAcquisition by adding their name to the JSON response.

From merge-ci: image

Before/after commit, JSON for the annotation link.parent:

Well:

"parent": {
    "id": 12992,
    "class": "WellI",
}

"parent": {
    "id": 12992,
    "class": "WellI",
    "name": "B2"
}

PlateAcquisition (case with name not set, as it's an optional property):

"parent": {
    "id": 11052,
    "class": "PlateAcquisitionI",
    "name": null
}

"parent": {
    "id": 11052,
    "class": "PlateAcquisitionI",
    "name": "Run 11052"
}
will-moore commented 7 months ago

This is working now for Plates/Wells/Images... Screenshot shows selection of Images - right panel shows KVP on Images, Wells and Plate:

Screenshot 2023-12-06 at 14 46 27

Just a couple of minor points about tooltips...

There's a missing line-break after the 1st row of the tooltips. Well ID: ... should be on it's own line.

The Inherited By section is shown in the first screenshot below, and it's useful because it tells you which Images the KVP applies to. But it's not shown for the 2nd screenshot where I've only selected a single Image from each Well. I think it would still be useful here.

Screenshot 2023-12-06 at 15 13 01

Screenshot 2023-12-06 at 15 13 25

Tom-TBT commented 7 months ago

Hello Petr, thank you for the input, I modified the tooltip accordingly.:

One object selected, no "inherited by" section: image

More than one object, selected, "inherited by" section: image

"inherited by" also shows when the annotations are inherited by a single object (as formulated in the previous request from Will): image