Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: Dynamic rechunking option for
StoreToZarr
#546WIP: Dynamic rechunking option for
StoreToZarr
#546Changes from 7 commits
6346d41
089dc7a
5fadc03
d0f3bcb
1903ca7
62d46ad
243c5c3
4ba4066
9375136
6b89214
f6e86d6
b2c48bf
b99c4ef
bc980f7
29097e8
f3aeaa5
c913d3b
9a845a1
39ff7e6
f608686
e3d61eb
d0fce99
ee45258
e97c492
f5bcea8
427dbe7
91eb6a6
a5a182a
707a9ca
4ad2a69
8df16ba
11de660
b8ead76
eecd8ed
c28ac56
7347c1b
96c57e8
edb4099
377133b
575bec2
9334e61
5062318
d0f2cf6
44aceaa
ca680c3
8d32404
53b0ae9
3de3168
db0551f
27bfd36
3ca76df
4838c5f
59432fd
d9115f7
ec8ab95
25823e6
ad79726
2cc6700
db6767b
fa05c9c
082979b
e40eb55
774fff8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted to review this PR but realized I was missing some key context. Could we provide a docstring for this function which explains what this function does and what these parameters are? In particular, I don't understand
target_chunk_ratio
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I was also confused by that term. So +1 on additional context. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be slightly clearer to call it
target_chunk_aspect_ratio
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a docstring explaining the inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially a good use case for hypothesis - you could parameterize a unit test with a hypothesis strategy that generates arbitrary (regular) chunks, then assert that the property that the returned target chunk size is within the specified tolerance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that is a good idea. I will have to dig into hypothesis a bit to understand how to encode the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR to dask might give you what you need (I can also finish it if that would help)
dask/dask#9374
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough tests.
Question: is there a reason you chose to use pytest's test class approach here? I'm not categorically opposed to it, but AFAICT the tests added by this PR would be the only class-based tests in pangeo-forge-recipes, so for stylistic consistency maybe better to write them in the function-based style, unless there's a specific reason not to?