Skip to content

Conversation

@will-moore
Copy link
Member

@will-moore will-moore commented Jun 21, 2021

See https://forum.image.sc/t/omero-workshop-data-preparation-idr0021/54100/5

The idr0021-data-prep.md document is quite out of date and the workflow has been simplified by more recent scripts.
The idr0021.groovy script now does everything in one step, although it doesn't add Map Annotations, but I don't think these are needed for the parade workshop?

This updates the workflow and updates a couple of scripts involved to use the cli login instead of entering login via script arguments.

To test the scripts, follow the 2 commands at the end, using the Plate ID from the first as input to the second:

$ python maintenance/scripts/idr_copy_plate.py 422
...
Created Plate: 754
$ python maintenance/scripts/channel_minmax_to_table.py 754

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-workshop-data-preparation-idr0021/54100/7

@stephenogg
Copy link

Hey Will -
Thanks for updating the instructions. I used them and they work great now. There's no more mention of the pesky image that's a Z-stack, NEDD1ab_NEDD1141_I_012_SIR. I had already deleted that image from my collection. I'm not sure if you need to add that sentence back to the idr0021-data-prep.md page?

I can also confirm that your version of channel_minmax_to_table.py worked for me without any issues too.

Thanks again,
Steve

@will-moore
Copy link
Member Author

Thanks @stephenogg - I didn't delete the NEDD1ab_NEDD1141_I_012_SIR image when testing locally and it still worked OK for parade, but you end up with multiple rows in the table for that image and it's kinda messy, so I think I'll add the delete step back in. Thanks!

@pwalczysko
Copy link
Member

python maintenance/scripts/idr_copy_plate.py 422

I have actually a question Re: cli_login. In the command above, I would like to copy the plate in a group which is not default. Thus, I would like to pass -g but the cli_login is not asking me about that...
I would have to edit the script now in order to copy into another group ? cc @joshmoore @sbesson

@sbesson
Copy link
Member

sbesson commented Aug 24, 2021

@pwalczysko: have you tested a 2-step workflow creating a session and re-using it?

omero login -g GROUP USER@SERVER
python maintenance/scripts/idr_copy_plate.py 422

Otherwise, I assume the script might need to be refactored to add optional login arguments and pass them to cli_login.

-    with cli_login() as cli:
+    with cli_login(login_args) as cli:

Is this a new feature as I do not see any non-default group support in the previous version of the script?

@pwalczysko
Copy link
Member

pwalczysko commented Aug 24, 2021

Is this a new feature as I do not see any non-default group support in the previous version of the script?

Previously, I would have used the 2-step workflow as suggested by yourself above, thanks. It was not clear to me that the 2-step workflow is still an option, even with the cli_login

@sbesson
Copy link
Member

sbesson commented Aug 24, 2021

Understood. cli_login() effectively calls omero login from within a script (with a contextmanager decorator) and it will use the same logic when it comes to re-using active OMERO.py sessions

@pwalczysko
Copy link
Member

Both scripts work as described, including the workaround suggested by @sbesson in #89 (comment), thanks

Will read the README now


print("table closed...")
orig_file = table.getOriginalFile()
table.close()
Copy link
Member

Choose a reason for hiding this comment

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

so the table could now never be closed if an error is thrown during the save step below

Copy link
Member Author

Choose a reason for hiding this comment

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

It was also possible before that it didn't get closed if an exception was thrown between table creation and closing.
But yes, a try/finally would be safer. I'll add that...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 9632c0d

@pwalczysko
Copy link
Member

Line 54 of the .md doc is not accurrate

Edit the ```project_id``` and run the [channel_names_from_maps.py](../scripts/channel_names_from_maps.py)

the user does not need to edit anything (indeed, there is not much to edit) as the script requires the ID as a parameter on input.

@pwalczysko
Copy link
Member

Retested the script channel_names_to_table.py as there are 2 new commits. Still works fine.

@pwalczysko
Copy link
Member

Two reformulations suggestions

  1. Fix parade prep notes #89 (comment)
  2. Fix parade prep notes #89 (review)

Otherwise ready to merge fmpov

@will-moore
Copy link
Member Author

Thanks @pwalczysko Those fixes in now.

@pwalczysko
Copy link
Member

Sorry, when checking your fixes, which are okay, found out that the line

Edit the script [idr_get_map_annotations.py](../

in this paragraph has the same problem as the one you just fixed - the script is not to be edited, as the IDs are passed as params. Cannot make a direct commit suggestion, as this is a line which you did not change in the PR.

@pwalczysko
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants