-
Notifications
You must be signed in to change notification settings - Fork 10
Add extinfo command #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add extinfo command #167
Conversation
03bde3a to
5f3f675
Compare
|
💯 As a part of this I can imagine wanting to |
|
|
Interesting... is this a python version thing? I use 3.12.9. |
|
It could be, I'm on an old env python 3.9.15. |
|
Yes, looks like it's supported from 3.10 on. As we still recommend 3.9 I'll remove that stuff again. |
src/omero_zarr/extinfo.py
Outdated
| select e from ExternalInfo as e | ||
| where e.id = :id | ||
| """ | ||
| extinfo = conn.getQueryService().findByQuery(query, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In think this needs to be a cross-group query findByQuery(query, params, conn.SERVICE_OPTS) otherwise you won't see it if the extinfo isn't in your default group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still outstanding. Same for the other conn.getQueryService().findByQuery below too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I do that one line above conn.SERVICE_OPTS.setOmeroGroup("-1") , is that not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, then you have to pass conn.SERVICE_OPTS that into the function call again. Ok, i'll fix that.
|
|
||
| POLYGONS_HELP = """Export ROI Polygons on the Image or Plate in zarr format""" | ||
|
|
||
| EXTINFO_HELP = """Get the ExternalInfo path of an ome.zarr image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get or set...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get, because that's the default action.
|
I was trying to test this on a non-zarr image, by specifying the It seems that there's something strange about |
|
There's a check if it is a ome.zarr: https://github.com/ome/omero-cli-zarr/pull/167/files#diff-2d64f99479103ff3050c633344ddf895f61491094e9dab65218a3ec17cbfcb59R177 As that's actually only intended to be used with zarr. Otherwise it wouldn't make sense to have it as omero-cli-zarr plugin!? |
|
The point is that you have a but it doesn't work for me. Does it work for you? |
3cc6c05 to
ad4a5b2
Compare
| hooks: | ||
| - id: flake8 | ||
| additional_dependencies: [ | ||
| flake8-blind-except, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too re-enable, we'll probably need help from @will-moore on knowing what exceptions can be thrown.
| flake8-blind-except, | ||
| flake8-builtins, | ||
| flake8-rst-docstrings, | ||
| flake8-logging-format, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it broke in Python 3.12: pytest-dev/pytest-testinfra#765
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over at https://github.com/ome/omero-cli-zarr/pull/168/files#diff-63a9c44a44acf85fea213a857769990937107cf072831e1a26808cfde9d096b9R56 I fixed this by including setuptools as dependency.
That is now conflicting with this PR.
I wonder if you could review that PR @dominikl or @joshmoore and we can get it merged (if that fix looks good)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I temporarly close this PR and then rebase it after #168 is merged, would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to close this one. Just merge in origin/master (and fix conflicts) once #168 is merged
|
Thanks Josh for fixing the precommit stuff! So this now suppports single image ome.zarrs, multi-series zarrs, and plates. Did I forget another use case? |
|
Thanks. A few flake8 and mypy failures at https://github.com/ome/omero-cli-zarr/actions/runs/15108557228/job/42462569460?pr=167 |
|
Changing to draft PR. Probably not gonna be merged, because this functionality will have to go somewhere else (possible |
e9aff8d to
a88d9b8
Compare
|
cf a88d9b8 I am not keen on the fake option. I think we should focus on creating an XML file. |
|
I'd just need it to test a zarr plate. Setting up a fake file for it would be much quicker/easier than a full metadata.ome.xml. |
|
it is an easy way to get data in but the metadata in the xml file are then read and handle by the XML reader stored accordingly. Such support does not exist in the fake reader. It will be preferable to handle as much metadata as possible and not look at a quick access in my opinion |
|
Quick discussion this am post standup: We will look at the Python snippet to generate the ome-xml file see https://github.com/IDR/idr0167-li-cellcyclenet/blob/main/scripts/ome-zarr-to-ome-xml.py |
f9c911a to
b558927
Compare
for more information, see https://pre-commit.ci
|
The extinfo command can now be used for different scenarios:
Same for image zarrs. |
This command will automatically set the ExternalInfo for ome.zarr images imported using the omero-zarr-pixel-buffer, if the
--setoption is specified. Default is to just print the ExternalInfo.I takes the path from the Fileset "usedFiles", which is assumed to only have one entry
..../SOME.ome.zarr/OME/METADATA.ome.xmland then set the ExternalInfo lsid to the full path of the multiscale 5d image;..../SOME.ome.zarr/0for an image..../SOME.ome.zarr/A/1/1etc. for plates.Limitations: Only works for ome.zarr imported from the filesystem.