-
Notifications
You must be signed in to change notification settings - Fork 10
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
Update ReadMe and Change Log #73
Conversation
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.
Looks good 👍
I remember it was mentioned that we should highlight the one change in behaviour that users might notice: labels are not now considered as separate images. I guess this maybe falls into docs rather than changelog (along with the new bfoptions documentation)? |
Yeah, that is a good point. I have added some additional description for that particular setting to note the change in behaviour |
Am I right in thinking that the way ZarrReader looks for Looking at that page, I don't see that either of these methods is mentioned for specifying bfoptions? |
Yeah, that is a good point, I will update the readme to include some details on the bfoptions file usage |
README.md
Outdated
| --- | --- | --- | | ||
| `omezarr.quick_read` | false | Improves the read performance by limiting the number of files that are parsed. This assumes that the shape and resolution count of all images in a plate remains constant | | ||
| `omezarr.save_annotations` | false | Determines if all the Zarr JSON metadata should be stored as XML annotations in the OME Model | | ||
| `omezarr.list_pixels` | false | Used to decide if getUsedFiles should list all of the pixel chunks | |
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 thought the default value for list_pixels
was true
since that's what you get when you import a zarr (with no bfoptions). Then we set it to false
with bfoptions
for IDR?
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.
Yeah thats correct, good catch, updated in the latest commits.
README.md
Outdated
|
||
The OMEZarrReader has a number of reader specific options in version 0.4.0 which can be used to customise the reader behaviour. This options can be used in the same manner as the reader options for Bio-Formats outlined [here](https://bio-formats.readthedocs.io/en/latest/formats/options.html#usage). | ||
|
||
The new default behaviour of the `omezarr.include_labels` option introduced in v0.4.0 represents a change in behaviour from the v0.3 releases. Previously any Zarr arrays found in the labels folder would by default be represented as an additional image series. With the current default settings, Zarr arrays in the labels folder will no longer be included in the list of image series. Changing this setting to `true` will revert to the previous behaviour. |
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.
If a user has previously imported images with labels using older ZarrReader, I guess that the images will break once they update ZarrReader?
This is probably quite uncommon - maybe we help users deal with this on a case-by-case basis, but maybe a warning of this could help?
I think the fix will involve adding bfoptions to the Fileset as I did at IDR/idr-metadata#684 (comment) - which is a bit painful psql etc. Any other options to workaround this issue?
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.
Yeah, certainly in the OMERO case, if files had been imported with and older version then they would either need to reimport or use the bfoptions. Realistically I don't know of anyone other than IDR that would have been impacted here, but if needs be we could assist with psql for adding bfoptions to the fileset. I have added an extra warning for this scanrio.
README.md
Outdated
|
||
The new default behaviour of the `omezarr.include_labels` option introduced in v0.4.0 represents a change in behaviour from the v0.3 releases. Previously any Zarr arrays found in the labels folder would by default be represented as an additional image series. With the current default settings, Zarr arrays in the labels folder will no longer be included in the list of image series. Changing this setting to `true` will revert to the previous behaviour. | ||
|
||
**Note:** If you had imported data with labels using version v0.3 or earlier then you will need to ensure that the `omezarr.include_labels` option is set to true. You can do this either via the API or by adding a `bfoptions` file to the fileset. In the case of OMERO this will require running psql commands to update the database to include the new `bfoptions` file. |
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.
Agreed that I think this is edge-case and don't expect anyone to need this but...
Is there any option to do this via the API when in OMERO? This paragraph is only about OMERO, right?
Maybe start with "If you had imported data with labels into OMERO...
.
Then you wouldn't need "In the case of OMERO..."?
Also I wonder if you should say "Please contact us on image.sc...." or something like that, otherwise it reads as if a user should know what psql commands to run.
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.
Yeah the API would not be for OMERO, I will make that paragraph OMERO specific and add the image.sc reference
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.
Looks great, thanks
I have updated the ReadMe to remove the outdated TODO section and add documentation for the new reader options. I have also added a link to the release on artifactory
The change log has also been updated to included missing entries from previous releases
Fixes #72