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

add init_array, and data kwarg for create_array #2761

Merged
merged 11 commits into from
Jan 29, 2025

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jan 24, 2025

Moves much of the logic currently contained in create_array down to a lower-level function called init_array, that creates array metadata + saves it to storage.

By popular request, I added a keyword argument to create_array: data, that allows users to provide an array-like object (default is None), from which dtype and shape can be inferred. I'm not a fan of what this does to the function signature, but at lot of people wanted this 🤷‍♂️ dtype and shape now default to None. If you specify data and dtype, then you get a warning about dtype being ignored in favor of data.dtype, and similarly for shape.

In parallel with #2622, I also added a write_data kwarg that controls whether data is written to the freshly created array. The default value here is True, which IMO is unsafe, but also requested by users.

As an indulgence. I also made some minor changes to the type aliases for ChunkKeyEncoding -- what we were calling ChunkKeyEncodingLike was actually a typeddict that should be ChunkKeyEncodingParams, and ChunkKeyEncodingLike should be ChunkKeyEncoding | ChunkKeyEncodingParams.

closes #2707, and partially addresses concerns raised in #2689

TODO:

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

@d-v-b d-v-b marked this pull request as ready for review January 24, 2025 16:42
@martindurant
Copy link
Member

Thank you

@d-v-b d-v-b requested a review from jhamman January 24, 2025 17:21
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 24, 2025
@d-v-b d-v-b requested a review from normanrz January 24, 2025 17:21
@dstansby
Copy link
Contributor

Nice! I think it would be helpful to not hide away the change to ChunkKeyEncoding here, and put it in it's own PR with it's own bugfix release notes entry. (and that would make the two PRs easier to review too)

@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 24, 2025

Nice! I think it would be helpful to not hide away the change to ChunkKeyEncoding here, and put it in it's own PR with it's own bugfix release notes entry. (and that would make the two PRs easier to review too)

good idea, I'll split it out

@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 25, 2025

#2763 should go in before this

@d-v-b d-v-b mentioned this pull request Jan 27, 2025
6 tasks
@d-v-b d-v-b requested a review from martindurant January 29, 2025 17:09
@martindurant
Copy link
Member

The tests look good.
The code diff is quite large for the change - I don't know when I'll have a time to go through it.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 29, 2025

tests are actually incomplete -- I didn't test the synchronous version of create_array (it doesn't take a data kwarg yet)

@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 29, 2025

I will get that in this evening

@d-v-b d-v-b removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 29, 2025
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 29, 2025
@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 29, 2025

@dstansby any idea why the release notes bot does not believe that I have added release notes? Is there something messed up with my release notes?

src/zarr/core/array.py Outdated Show resolved Hide resolved
@d-v-b d-v-b merged commit fc08f31 into zarr-developers:main Jan 29, 2025
30 checks passed
@d-v-b d-v-b deleted the feat/init_array branch January 29, 2025 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs release notes Automatically applied to PRs which haven't added release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-add data= in zarr.core.array.create_array
4 participants