Skip to content
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

write_multiscales does not expose additional metadata #176

Closed
constantinpape opened this issue Mar 16, 2022 · 7 comments · Fixed by #178
Closed

write_multiscales does not expose additional metadata #176

constantinpape opened this issue Mar 16, 2022 · 7 comments · Fixed by #178

Comments

@constantinpape
Copy link
Contributor

write_multiscales does not expose additional metadata as a argument, which could be used to e.g. add the name field.
Note that this is supported by write_image. I think that write_multiscales should be updated s.t. it also supports this.

@constantinpape
Copy link
Contributor Author

@sbesson @will-moore @joshmoore
this would be a very easy change, I am happy to make a PR for this if you agree

@sbesson
Copy link
Member

sbesson commented Mar 16, 2022

From my side, this proposal largely overlaps with #171 (comment). 👍 for adding the API allowing to to store extra attributes at the write_multiscales level.

I briefly looked at it in the context of #171 but I held off as I stumbled upon an API style question which I could not resolve (and did not capture):

It looks like there is room for unification here. The biggest issue I see with the second form would be the potential collision with other arguments. @glyg @satra @joshmoore

The concern above might not be an immediate concern and I am happy to capture it as a separate issue. I just wanted to raise it before we start expanding the API in case you foresee it being a problem for your use case.

@constantinpape
Copy link
Contributor Author

Thanks for the comment @sbesson. I agree that there is a bit of a tension between just adding kwargs and adding new special purpose fields for things like labels etc.

My initial thought would be the following: add metadata kwargs to write_multiscales etc. and then add another very thin wrapper like add_image_label_multiscales that has extra fields for the relevant label metadata.

For now this issue is not super urgent for my use-case since I added a work-around: https://github.com/ome/ome-ngff-prototypes/blob/main/workflows/spatial-transcriptomics-example/convert_transcriptomics_data_to_ngff.py#L44-L48.

@sbesson
Copy link
Member

sbesson commented Mar 16, 2022

My initial thought would be the following: add metadata kwargs to write_multiscales etc. and then add another very thin wrapper like add_image_label_multiscales that has extra fields for the relevant label metadata.

So is this effectively addressing #171? Or are there more steps to solve this use case?

@constantinpape
Copy link
Contributor Author

constantinpape commented Mar 17, 2022

So is this effectively addressing #171? Or are there more steps to solve this use case?

So I think we would need two things to address this:

  • same function signature for write_image and write_multiscales that accepts **metadata
  • a wrapper function write_image_labels, that uses write_image internally and sets some of the additional label metadata (also on the group level). One could also add write_multiscale_image_labels that mirrors write_multiscales.

I can take a shot at this if this sounds like a good plan to you, @will-moore @joshmoore @sbesson.

@sbesson
Copy link
Member

sbesson commented Mar 17, 2022

The progression expressed above makes sense to me.
Barring the concerns expressed in #176 (comment), the first API addition looks straighforward and non controversial since write_image already supports **kwargs. For the second one, I can imagine there are a few ways to construct the API.
Happy to give these a review.

@constantinpape
Copy link
Contributor Author

Great! I will give it a go later.

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 a pull request may close this issue.

2 participants