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

Parse chunk shape to check for float values #2535

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

faymanns
Copy link
Contributor

@faymanns faymanns commented Dec 5, 2024

This PR solves the fact that specifying a chunk shape containing one or several float values does not raise an error.
Before the PR the values after the decimal point are trimmed without a warning.
After the PR a TypeError is raised when the chunk shape contains float values.
It solves issue #2530.

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/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

Copy link
Contributor

@d-v-b d-v-b left a comment

Choose a reason for hiding this comment

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

looks good, thank you

Comment on lines 464 to 468
chunks = parse_shapelike(chunks)
_chunks = normalize_chunks(chunks, shape, dtype_parsed.itemsize)
else:
if chunk_shape:
chunk_shape = parse_shapelike(chunk_shape)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we move this check into normalize_chunks. Here's where the behavior you're looking to change is:

if isinstance(chunks, numbers.Integral):
chunks = tuple(int(chunks) for _ in shape)

If you look at normalize_chunks, you'll see why Zarr is more permissive on chunk shape than array shape. I'm not opposed to us disallowing floats but we need to be careful keep some of the other behaviors in normalize_chunks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems like a better place indeed! I've updated the PR.

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.

Thanks @faymanns for working on this. I left one comment to direct your attention to normalize_chunks

@jhamman jhamman enabled auto-merge (squash) December 5, 2024 18:34
@jhamman jhamman merged commit fd688c4 into zarr-developers:main Dec 5, 2024
22 checks passed
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 this pull request may close these issues.

3 participants