-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add support for label properties in zattrs and plugin #61
Conversation
ome_zarr/reader.py
Outdated
label_val = props['label-value'] | ||
del props['label-value'] | ||
properties[label_val] = props |
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.
Could you explain what's happening here? We'll need to check whether the underlying dictionary is a deep-copy of the original, or whether it returns a reference which means this may update the properties seen by other code.
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.
Yes, if I do image_label.get('properties', [])
after this for
loop, I get the properties without the label-value
s so you probably should create a copy of the props
before editing.
This seems to work:
properties[label_val] = dict(props)
del properties[label_val]['label-value']
ome_zarr/napari.py
Outdated
# napari expects lists of equal length so we must fill with None | ||
for prop_key in all_keys: | ||
reader_props[prop_key] = [ | ||
props[i][prop_key] if prop_key in props[i] else "None" |
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 can probably be an actual None
value here instead of a string "None"
(unless it has to be a string for some reason). Since if some code is testing for the value being None
, then the string won't be recognised as None
.
Pushed a commit to update the miniconda action since it looks like your PR was running into:
|
Thanks for this, pre-commit was giving me the run around... |
…a/ome-zarr-py into add-label-properties
@joshmoore I've added a test to Aside from tests I've now also added a PR for the spec update - I hope that's the right approach. What else needs doing to fully incorporate the label properties into the package? |
Ah, @DragaDoncila. Sorry, I see I owe(d) you a comment regarding the spec & more tests. Your PR against omero-ms-zarr was perfect and against the right repo at the time. 💯 You saw that I migrated it. Further PRs welcome against ome/zarr. 😄 Regarding tests, I imagine there will need to be a new file created for what you were wanted to accomplish. Feel free to open a new PR if you're interested. All the best, ~Josh Merging along with ome/ngff#3 |
Add support for label properties of the form
as discussed in #60 .
Read these properties in as dictionary of
label-value
to properties.Transform these properties in the napari reader to:
This is the very first pass, so would appreciate feedback on the general approach before I move on to updating spec, documentation, tests, etc.
We probably also wish to consider what types of property keys and values we want to accept.