Move_Annotations #134

will-moore · 2017-03-03T15:11:33Z

Script to Select Screens, Plates, or Wells and move Annotations from Images onto their parent Wells.
See https://trello.com/c/gasafKWk/291-update-script-wellsample-well-annotation-migration

To test:

Add a bunch of Annotations to Images in Wells. Pick some Wells that have multiple fields.
- Add tags, ratings, map annotations, comments etc.
Select Screens, Plates or Wells and run script (check that selected S/P/W are populated in dialog)
Choose to filter annotations by Type or Namespace. (can test this using openmicroscopy.org/omero/client/mapAnnotation for Map Annotations).
Test "Remove Annotations" removes annotations from Images if checked.

jburel · 2017-03-04T21:10:30Z

omero/util_scripts/Move_Annotations.py

+    """Move annotations from Images in this Well onto the Well itself."""
+    print "Processing Well:", well.id, well.getWellPos()
+    iids = [wellSample.getImage().id for wellSample in well.listChildren()]
+    print "  Image IDs:", iids


I have remove print from all other scripts.
Could you remove it

Ahhhhh! The prints for scripts provide the log for the user. If you don't have any prints then they won't know what's gone wrong or won't have any "info" on what the script has done. They are essential.
At least where I've used a log() function to do the printing, it's only a case of adding back the print statement there, but for scripts with individual print statements this will be a fair bit harder.

for now I will follow what has been done in the export scripts i.e. log and do a print at the end if required (that could be turned into an option)
and many of those prints are not necessary
they are helpful when debugging.
We will have to review that part in another round (analyis scripts initialize a logger that is not used)

jburel · 2017-03-04T21:10:52Z

omero/util_scripts/Move_Annotations.py

+
+    new_links = []
+    for l in old_links:
+        print "    Annotation:", l.child.id.val, l.child.__class__.__name__


jburel · 2017-03-04T21:11:15Z

omero/util_scripts/Move_Annotations.py

+    try:
+        conn.getUpdateService().saveArray(new_links)
+    except Exception, ex:
+        print "Failed to create links: ", ex.message


same. Log error instead

jburel · 2017-03-04T21:11:20Z

omero/util_scripts/Move_Annotations.py

+        return 0
+
+    if remove_anns:
+        print "Deleting ImageAnnotation links...", link_ids


jburel · 2017-03-04T21:11:31Z

omero/util_scripts/Move_Annotations.py

+                to_delete = ImageAnnotationLinkI(link_id, False)
+                conn.getUpdateService().deleteObject(to_delete)
+        except Exception, ex:
+            print "Failed to delete links: ", ex.message


jburel · 2017-03-04T21:12:59Z

omero/util_scripts/Move_Annotations.py

+    return ann_total
+
+
+def runScript():


run_script to match the formatting used in the other script

jburel · 2017-03-04T21:13:29Z

omero/util_scripts/Move_Annotations.py

+
+    client = scripts.client(
+        'Move_Annotations.py',
+        """Move Annotations from SPW Images to their parent Wells.""",


I won't use SPW, better to be explicit

jburel · 2017-03-04T21:14:38Z

omero/util_scripts/Move_Annotations.py

+
+        scripts.List(
+            "IDs", optional=False, grouping="2",
+            description="List of Screen IDs or Plate IDs").ofType(rlong(0)),


well is supported as a data_type, this need to be reflected in the description of the ids param

jburel · 2017-03-04T21:15:10Z

omero/util_scripts/Move_Annotations.py

+        scripts.String(
+            "Annotation_Type", grouping="3",
+            description="Move All annotations OR just one type of annotation"
+            " Z below.", values=ann_types, default='All'),


What's Z below?

jburel · 2017-03-04T21:15:30Z

omero/util_scripts/Move_Annotations.py

+        conn = BlitzGateway(client_obj=client)
+
+        script_params = client.getInputs(unwrap=True)
+        print script_params


same please remove

jburel · 2017-03-04T21:18:15Z

Since this will be an official test, an integration test must be added to https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/test/integration/scriptstest/test_util_scripts.py

jburel · 2017-03-04T21:19:01Z

./omero/util_scripts/Move_Annotations.py:49:29: E128 continuation line under-indented for visual indent ./omero/util_scripts/Move_Annotations.py:123:5: N802 function name should be lowercase ./omero/util_scripts/Move_Annotations.py:184:1: E305 expected 2 blank lines after class or function definition, found 1

jburel · 2017-03-04T21:21:11Z

note that on this repo "flake8" is more "aggressive" than on the openmicroscopy repo
i.e. flake8 pycodestyle pep8-naming are installed

mtbc · 2017-03-05T17:31:54Z

Would it be possible to avoid attempting to link when such a link already exists? Might be especially useful for trying to re-run a script after deleting old links failed.

mtbc · 2017-03-05T17:33:56Z

If an admin runs the script would it be worth trying to preserve the ownership of the previous link?

mtbc · 2017-03-05T17:35:03Z

omero/util_scripts/Move_Annotations.py

+        try:
+            for link_id in link_ids:
+                to_delete = ImageAnnotationLinkI(link_id, False)
+                conn.getUpdateService().deleteObject(to_delete)


could delete in bulk?

jburel · 2017-03-05T17:43:45Z

@mtbc: I think we have to preserve the ownership of the links

joshmoore · 2017-03-08T07:54:55Z

NB: This seems like code ripe for wrapping into a server-side method: "relink". cc: @mtbc

mtbc · 2017-03-08T08:23:33Z

Certainly feasible. If it might be done enough in general to warrant server-side support, might be worth a design issue to discuss exactly what it should be able to do and what use cases should be supported.

mtbc · 2017-03-08T10:31:29Z

I'm curious about users in read-only groups running this on others' data: what happens?

Also, can I make a plate from someone else's dataset in a read-only group? What happens then when the annotations are being moved from a same-owner link to a different-owner link (whether by the plate owner or the image owner)?

mtbc · 2017-03-08T10:39:13Z

Would a dry-run mode be useful?

will-moore · 2017-03-08T14:13:42Z

After discussion with @pwalczysko and @mtbc, we decided to only move links that you own (unless you are an Admin).
This simplifies lots of questions about whether to delete another owner's links that you have moved, how to distinguish between new links that were generated from your own links vv generated from someone else's links, whether the script should behave differently in read-write vv read-ann group etc.

Also, we now check for existing links, so we should avoid ValidationExceptions (e.g. if you run the script twice without removing annotations).

pwalczysko · 2017-03-09T10:55:28Z

Remove Annotations --> Remove Annotations from Images (first thought this would mean completely remove all annotations and do nothing else (i.e. do not add them to wells). If you add ...from Images than it is clearer you are not talking about complete wipeout of the annotations.

pwalczysko · 2017-03-09T12:31:31Z

RFE: put some helpful icons into activities which would lead you to the well with the freshly attached annotations (just like for the other scripts).

pwalczysko · 2017-03-09T13:25:25Z

I don't think that this box should be ticked by default.

Especially considering that you can re-run the script a second time with the same settings, only ticking the box and the annotations will still be removed from the images.

Thus, the user will be hopefully inclined to run the script the first time, not deleting anything, when they are happy and have checked, they might want to run the script the second time with the box ticked.

will-moore · 2017-03-09T13:50:51Z

@jburel What do you think to @pwalczysko's suggestion that "Remove Annotations" should not be checked by default?

jburel · 2017-03-09T13:57:05Z

I will agree with @pwalczysko
since we have moving towards "dry-run" for all actions, I will say not ticked by default too

pwalczysko · 2017-03-09T14:22:15Z

Tested up till now:

work on my own data ony
- checked all types of annotations (Tables are not an annotation in true sense, not tested as there is no object in the back end to manipulate)
- checked the checkbox "Remove annotations"
- checked filtering for Map Annotations (namespace and Type)
- checked selecting Well and Plate (it does all wells in that Plate)
- ToDo: Check selecting Screen with multiple Plates
ToDo: work as an admin on other's data (expected behaviour is: all the new links belong to the owners of the corresponding original links)
ToDo: work on your own data, but with somebody else annotating them as well (expected behaviour is: only your own links will be transferred, the other user's links will be left as they are)

dominikl · 2017-03-09T15:04:27Z

Looks good, tested (locally) with Screen, Plate and Well <-> Tags, Map, File, Rating, Comment. Only own annotations get moved, unless run as admin (in which case original ownership is preserved) 👍

mtbc · 2017-03-10T09:28:23Z

omero/util_scripts/Move_Annotations.py

+    'File': 'FileAnnotationI',
+    'Comment': 'CommentAnnotationI',
+    'Rating': 'LongAnnotationI',
+    'Key-Value': 'MapAnnotationI'


This subset is basically those that the web UI will usefully display for wells?

Yes. My assumption is that users who have added annotations via their own scripts / clients will either have given them a useful namespace that can be used to select them in the Move_Annotations script OR they can also write their own script (or update this one) to customise the task.
Although I could add extra types here, I think this adds extra complexity for users and more testing for us which is probably not justified at this stage.

Fair enough, thank you.

mtbc · 2017-03-10T09:29:50Z

omero/util_scripts/Move_Annotations.py

+
+        scripts.String(
+            "Annotation_Type", grouping="3",
+            description="Move All annotations OR just one type of annotation",


why is "All" capitalized?

Ooops - getting a touch of Title Case here. https://medium.com/@jsaito/making-a-case-for-letter-case-19d09f653c98#.h1s476nc6. I'll fix...

I guess it's because "All" is an option in the dropdown list

pwalczysko · 2017-03-10T10:46:45Z

Retest in UI went fine, finished all the ToDo's from #134 (comment) as well as checked the new label on the checkbox Remove.... and the new navigation tool in the activities, which works fine.

Ready to merge after the questions of @mtbc are answered.

will-moore · 2017-03-10T14:04:01Z

NB: I still need to finish the integration test for this script, and also add support for linking to Wells from the 'Browse' button in the Activities panel.

jburel · 2017-03-13T14:06:52Z

one RFE: Support for plate acquisition in the type

jburel · 2017-03-13T15:11:48Z

omero/util_scripts/Move_Annotations.py

+        try:
+            for link_id in link_ids:
+                to_delete = ImageAnnotationLinkI(link_id, False)
+                conn.getUpdateService().deleteObject(to_delete)


you should not use the deprecated method, use delete2 instead

jburel · 2017-03-14T06:50:32Z

./omero/util_scripts/Move_Annotations.py:24:1: F401 'omero.model.ImageAnnotationLinkI' imported but unused

jburel · 2017-03-15T12:07:57Z

https://trello.com/c/TNJk38iN/36-move-annotation-rfe to capture suggestions

jburel · 2017-03-16T15:59:18Z

Thanks for making the changes.
2 failures with the test currently

will-moore added 2 commits March 3, 2017 14:54

New Move_Annotations from Images to Wells script

5698c3f

Filter by namespace for Ratings

b3d690e

jburel reviewed Mar 4, 2017

View reviewed changes

mtbc reviewed Mar 5, 2017

View reviewed changes

will-moore added 5 commits March 5, 2017 23:18

Small fixes PR ome#134

5efb271

flake8 fix

aac5c51

Move print statements to use log()

07b0837

flake8

c7f6353

If user isAdmin(), preserve link ownership on move

0f51448

Move_Annotations.py only moves your own links, unless Admin

19b4379

pep8 fix

5eeec4d

Move_Annotations.py returns target object for UI

7f63ba2

parameter 'Remove_Annotations_From_Images' default is False

ccfe164

mtbc reviewed Mar 10, 2017

View reviewed changes

Fix 'All' -> 'all'

fd9b1b8

will-moore mentioned this pull request Mar 11, 2017

Test move annotations script ome/openmicroscopy#5177

Merged

jburel reviewed Mar 13, 2017

View reviewed changes

Use Delete2 to delete ImageAnnotationLinks

7994ac7

flake8 fix

5ff1cf2

jburel merged commit 4994916 into ome:develop Mar 17, 2017

jburel added this to the 5.3.0 milestone Mar 29, 2017

Move_Annotations #134

Move_Annotations #134

Uh oh!

Conversation

will-moore commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburel Mar 6, 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!

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!

jburel commented Mar 4, 2017

Uh oh!

jburel commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jburel commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtbc commented Mar 5, 2017

Uh oh!

mtbc commented Mar 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburel commented Mar 5, 2017

Uh oh!

joshmoore commented Mar 8, 2017

Uh oh!

mtbc commented Mar 8, 2017

Uh oh!

mtbc commented Mar 8, 2017

Uh oh!

mtbc commented Mar 8, 2017

Uh oh!

will-moore commented Mar 8, 2017

Uh oh!

pwalczysko commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwalczysko commented Mar 9, 2017

Uh oh!

pwalczysko commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

will-moore commented Mar 9, 2017

Uh oh!

jburel commented Mar 9, 2017

Uh oh!

pwalczysko commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

will-moore commented Mar 3, 2017 •

edited

Loading

jburel Mar 6, 2017 •

edited

Loading

jburel commented Mar 4, 2017 •

edited

Loading

jburel commented Mar 4, 2017 •

edited

Loading

pwalczysko commented Mar 9, 2017 •

edited

Loading

pwalczysko commented Mar 9, 2017 •

edited

Loading

pwalczysko commented Mar 9, 2017 •

edited

Loading

dominikl commented Mar 9, 2017 •

edited

Loading