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.

Sign up for GitHub

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

Jump to bottom

open_array(): adding the meta_array argument #1396

Merged
merged 8 commits into from
May 25, 2023
Merged

open_array(): adding the meta_array argument #1396

merged 8 commits into from
May 25, 2023

Conversation

madsbk
Copy link
Contributor

@madsbk madsbk commented Apr 20, 2023 •
edited
Loading

Like the other creation functions, open_array() now takes the meta_array argument.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Apr 20, 2023
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Apr 20, 2023
@codecov
Copy link

codecov bot commented Apr 20, 2023 •
edited
Loading

Codecov Report

Merging #1396 (ee6807b) into main (cc16d8c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1396   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        37           
  Lines        14857     14866    +9     
=========================================
+ Hits         14857     14866    +9
Impacted Files Coverage Δ
zarr/creation.py 100.00% <ø> (ø)
zarr/tests/test_meta_array.py 100.00% <100.00%> (ø)

@madsbk madsbk marked this pull request as ready for review April 20, 2023 15:21
@quasiben
Copy link

@rabernat i think this is the last piece needed for xarray/GDS support

@quasiben quasiben mentioned this pull request Apr 25, 2023
Closed
@madsbk madsbk mentioned this pull request Apr 28, 2023
Merged
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this pull request Apr 28, 2023
@madsbk
Zarr+nvCOMP (#208)
28e60bb 
Some quick numbers (all IO reads):

|       | Compression Ratio | Throughput
| ----------- | :-----------: |-----------: |
| POSIX                               | n/a      | 1.63 GiB/s |
| KvikIO (compat mode)  | n/a        |   8.43 GiB/s |
| Zarr (no compression)   | n/a        |8.00 GiB/s | 
| Zarr+Snappy | 1000/322        | 10.57 GiB/s|
| Zarr+Gdeflate | 1000/27        | 29.05 GiB/s|
| Zarr+LZ4 | 1000/9        | 48.95 GiB/s|


Notice, this is just a quick benchmark to show the potential of nvCOMP. 
When Zarr v2.15 is released and #131 merged, we can start optimizing. 
Additionally, when zarr-developers/zarr-python#1396 is merged, we can introduce a wrapper for `zarr.open()` that seamlessly reads/writes to/from GPU memory and do on-the-fly compression.

Authors:
  - Mads R. B. Kristensen (https://github.com/madsbk)

Approvers:
  - Benjamin Zaitlen (https://github.com/quasiben)

URL: #208
@quasiben
Copy link

quasiben commented May 1, 2023

Would it be possible to include this in the 2.15 Zarr release ?

@joshmoore
Copy link
Member

Makes sense to me to have it with your other PR, @madsbk. (Sidenote: 2.15a1 just went out)

@madsbk
Copy link
Contributor Author

madsbk commented May 2, 2023 •
edited
Loading

Makes sense to me to have it with your other PR, @madsbk. (Sidenote: 2.15a1 just went out)

Agree, it would be great to get this in v2.15.0!

@joshmoore
Copy link
Member

@madsbk: quick higher-level question -- how many other locations do you think might need this?

@madsbk
Copy link
Contributor Author

madsbk commented May 8, 2023

@madsbk: quick higher-level question -- how many other locations do you think might need this?

I am not sure, but with this PR merged all functions in creation.py accepts the meta_array argument.

@madsbk
Copy link
Contributor Author

madsbk commented May 17, 2023

Anything that needs to be done here?

@joshmoore
Copy link
Member

Hi @madsbk. Apologies. I took a look after your last push, but the Windows 3.11 build was failing and a re-run didn't fix it (and then I promptly got side-tracked).

I just re-ran and all is green. Let's get this in. I'll push out an immediate pre-release and then review the state of everything tomorrow. (Feedback on a full release of 2.15.0 welcome from any/all.)

Thanks for hanging in there!

@joshmoore joshmoore merged commit 3649b9b into zarr-developers:main May 25, 2023
@madsbk
Copy link
Contributor Author

madsbk commented May 26, 2023

Awesome, thanks @joshmoore !

@madsbk madsbk deleted the open_array_meta_array branch August 7, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@madsbk @quasiben @joshmoore