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

BUG: Fix chunk arguments for normalize_chunks #820

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

phofl
Copy link
Contributor

@phofl phofl commented Oct 31, 2024

  • Closes #xxxx
  • Tests added
  • Fully documented, including docs/history.rst for all changes and docs/rioxarray.rst for new API

The current specification of the previous chunks is incorrect, it always needs to sum up to the whole axis if given as tuples. What you want is an integer here.

I've recently refactored how normalize_chunks works and this will break things for you. I'll add some compat code for now, but will want to remove this at some point, so fixing it over here

@phofl
Copy link
Contributor Author

phofl commented Oct 31, 2024

I am not sure how and if we can test this unfortunately.

rioxarray/_io.py Outdated Show resolved Hide resolved
Co-authored-by: Alan D. Snow <[email protected]>
@snowman2
Copy link
Member

snowman2 commented Nov 1, 2024

This was added in #31. I am concerned about modifying code that has been around for so long without some way to test and verify that the behavior works as expected.

@phofl
Copy link
Contributor Author

phofl commented Nov 1, 2024

The previous way of specifying chunks was technically invalid, i.e. if you pass something like

chunks=((1, ), (1, ), (100, )) 
shape = (1, 10, 100)

to any array constructor (da.random.random for example), it will raise. The way this was intended was to pass (1, 1, 100) instead (historically, normalize_chunks treated both inputs equivalent). This PR addresses that.

Re testing: Every test in the test suite that uses chunks=True|"auto" tests this behaviour implicitly, so stuff would break if that would create an incompatible output.

I've put up a PR for Dask that makes both inputs equivalent for the time being and it's tested over there that both notations are equivalent.

@snowman2
Copy link
Member

snowman2 commented Nov 1, 2024

After looking at dask's tests, I agree with your conclusion. Thanks!

@snowman2 snowman2 changed the title Fix chunk arguments for normalize_chunks BUG: Fix chunk arguments for normalize_chunks Nov 1, 2024
@snowman2 snowman2 added this to the 0.17.1 milestone Nov 1, 2024
@snowman2 snowman2 added the bug Something isn't working label Nov 1, 2024
@phofl
Copy link
Contributor Author

phofl commented Nov 1, 2024

Great, thanks for reviewing!

@snowman2 snowman2 merged commit 94a80fd into corteva:master Nov 1, 2024
14 of 19 checks passed
@phofl phofl deleted the chunk-spec branch November 1, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants