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

Avoid redundant metadata reads in ZarrArrayWrapper #8297

Merged
merged 6 commits into from
Oct 13, 2023

Conversation

olimcc
Copy link
Contributor

@olimcc olimcc commented Oct 12, 2023

Modify ZarrArrayWrapper to avoid re-reading metadata each time data is read from the array.

Modify ZarrArrayWrapper to avoid re-reading metadata each time data
is read from the array.
@welcome
Copy link

welcome bot commented Oct 12, 2023

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

@github-actions github-actions bot added topic-backends topic-zarr Related to zarr storage library io labels Oct 12, 2023
@max-sixty max-sixty requested a review from rabernat October 12, 2023 17:11
Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion but this looks great. No doubt this is going to really improve performance when reading data with zarr.

Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, thanks!

Maybe some get_array() calls can be replaced by _array, but that's really minor.

You should add an entry in whats-new!

And as a bonus you could add a benchmark, but that's really optional (seems like there are only netcdf Io benchmarks right now).

@olimcc
Copy link
Contributor Author

olimcc commented Oct 13, 2023

Thanks @headtr1ck!

You should add an entry in whats-new!

Great, this has been done.

And as a bonus you could add a benchmark, but that's really optional (seems like there are only netcdf Io benchmarks right now).

I didn't do this. I'll try to add a note to #8290 of a before/after scenario, to illustrate the impact of the reduced requests.

@jhamman jhamman merged commit 5876365 into pydata:main Oct 13, 2023
26 checks passed
@welcome
Copy link

welcome bot commented Oct 13, 2023

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io topic-backends topic-zarr Related to zarr storage library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential performance optimization for Zarr backend
4 participants