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

feat(toMultiscaleChunkedImage): Support passing a Zarr store #391

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Mar 1, 2021

No description provided.

@thewtex thewtex requested a review from oeway March 1, 2021 20:43
@oeway
Copy link
Collaborator

oeway commented Mar 1, 2021

Hi @thewtex This is awesome, I am testing it now.

I noticed that you have these lines in the extractScaleInfo function:

const zattrs = await store.getItem('.zattrs')
const multiscales = zattrs.multiscales
const name = multiscales[0]['name']
const datasets = multiscales[0].datasets

Are you assuming the getItem will return an json object directly?
Shouldn't it always be an ArrayBuffer? I think that will simplify the implementation of store and make it key-agnostic. For example, this is a reference store implementation: https://gist.github.com/oeway/d8b5bad5cbb7bc234a87d2e8948d9164#file-vizarrreferencestore-imjoy-html-L49-L70

@thewtex
Copy link
Member Author

thewtex commented Mar 2, 2021

Are you assuming the getItem will return an json object directly?

Yes, a json object or an arraybuffer.

Shouldn't it always be an ArrayBuffer?

This makes things more complex and adds unnecessary conversions, correct? Why force JSON data into an ArrayBuffer? And then convert it back? With Zarr, we know that some keys are always JSON and some are always ArrayBuffer's.

@thewtex
Copy link
Member Author

thewtex commented Mar 2, 2021

While it seems to add unnecessary overhead, I'll change the behavior to be consistent with the Zarr Python spec and behavior, which is always using bytes for values.

@oeway
Copy link
Collaborator

oeway commented Mar 2, 2021

Yes, I agree that will be some overhead, but perhaps it doesn't matter that much because those are mostly for meta information which often very small.

I was about to say the same, since the Zarr store in Python also return bytes.

JSON Zarr members are also returned as ArrayBuffer's from the store, in
line wit the Zarr spec.
@thewtex
Copy link
Member Author

thewtex commented Mar 9, 2021

@oeway thanks for the review and suggestion. All ArrayBuffer values is now implemented. Please take a look.

@oeway
Copy link
Collaborator

oeway commented Mar 9, 2021

Hey, thanks a lot! Super exciting to try it!

The PR looks good! I just trie to test it but don't have the right data format yet.

@thewtex
Copy link
Member Author

thewtex commented Mar 9, 2021

Thanks! With the foundation down, I'll continue with the other store implementations :-)

@thewtex thewtex merged commit ab848ac into Kitware:master Mar 9, 2021
@thewtex thewtex deleted the image-store branch March 9, 2021 14:50
@github-actions
Copy link

github-actions bot commented Mar 9, 2021

🎉 This PR is included in version 11.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants