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 xr.unify_chunks() top level method #5445

Merged
merged 25 commits into from
Jun 16, 2021
Merged

Add xr.unify_chunks() top level method #5445

merged 25 commits into from
Jun 16, 2021

Conversation

malmans2
Copy link
Contributor

@malmans2 malmans2 commented Jun 6, 2021

  • Closes Add xr.unify_chunks top level method #3371
  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@pep8speaks
Copy link

pep8speaks commented Jun 6, 2021

Hello @malmans2! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-15 12:11:39 UTC

@malmans2 malmans2 changed the title Add top level method Add xr.unify_chunks() top level method Jun 6, 2021
doc/whats-new.rst Outdated Show resolved Hide resolved
xarray/core/dask_array_ops.py Outdated Show resolved Hide resolved
@jhamman
Copy link
Member

jhamman commented Jun 11, 2021

pinging @dcherian and/or @crusaderky who may have thoughts on this PR.

xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/tests/test_dask.py Show resolved Hide resolved
xarray/tests/test_dask.py Outdated Show resolved Hide resolved
malmans2 and others added 4 commits June 14, 2021 12:54
Co-authored-by: crusaderky <crusaderky@gmail.com>
Co-authored-by: crusaderky <crusaderky@gmail.com>
Co-authored-by: crusaderky <crusaderky@gmail.com>
Co-authored-by: crusaderky <crusaderky@gmail.com>
malmans2 and others added 3 commits June 14, 2021 13:08
Co-authored-by: crusaderky <crusaderky@gmail.com>
Co-authored-by: crusaderky <crusaderky@gmail.com>
@malmans2
Copy link
Contributor Author

Thanks @crusaderky! I think all your suggestions are now implemented.

@crusaderky
Copy link
Contributor

LGTM.
Note that the function doesn't align indices.
e.g. if you have:

a = DataArray([0,1,2,3], dims=["x"], coords={"x": [0,10,20,30]}).chunk(3)
b = DataArray([0,1,2,3], dims=["x"], coords={"x": [10,30,40,50]}).chunk(2)
a, b = unify_chunks(a, b)

You'll end up with aligned chunks, but not aligned coords (e.g. both outputs have still values=[0,1,2,3]), which doesn't make much sense.
I think this is OK to leave it as it is in this specific case the issue should not do much harm anyway and it would just be a slowdown in most cases; I'd like to hear @dcherian's or @jhamman's opinions though.

@dcherian
Copy link
Contributor

Ya I think it's OK for this function

@dcherian
Copy link
Contributor

Thanks @malmans2 and @crusaderky!

@dcherian dcherian merged commit fe87162 into pydata:master Jun 16, 2021
@github-actions
Copy link
Contributor

Unit Test Results

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit fe87162. ± Comparison against base commit fe87162.

@malmans2 malmans2 deleted the add_unify_chunks branch June 21, 2021 08:53
@malmans2 malmans2 restored the add_unify_chunks branch June 21, 2021 08:53
@malmans2 malmans2 deleted the add_unify_chunks branch June 21, 2021 08:53
@TomNicholas TomNicholas mentioned this pull request Jul 8, 2021
8 tasks
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.

Add xr.unify_chunks top level method
7 participants