ome / omero-scripts

Core OMERO Scripts
https://pypi.org/project/omero-scripts/
12 stars 32 forks source link

Move_Annotations #134

Closed will-moore closed 7 years ago

will-moore commented 7 years ago

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:

screen shot 2017-03-03 at 14 59 17

jburel commented 7 years ago

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 commented 7 years ago

./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 commented 7 years ago

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

mtbc commented 7 years ago

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 commented 7 years ago

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

jburel commented 7 years ago

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

joshmoore commented 7 years ago

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

mtbc commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Would a dry-run mode be useful?

will-moore commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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). screen shot 2017-03-09 at 12 29 51

pwalczysko commented 7 years ago

I don't think that this box should be ticked by default. screen shot 2017-03-09 at 13 24 13

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 commented 7 years ago

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

jburel commented 7 years ago

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

pwalczysko commented 7 years ago

Tested up till now:

dominikl commented 7 years ago

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) 👍

pwalczysko commented 7 years ago

Retest in UI went fine, finished all the ToDo's from https://github.com/ome/scripts/pull/134#issuecomment-285363851 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 commented 7 years ago

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 commented 7 years ago

one RFE: Support for plate acquisition in the type

jburel commented 7 years ago

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

jburel commented 7 years ago

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

jburel commented 7 years ago

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