-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
ENH: use dask.array.apply_gufunc
in xr.apply_ufunc
#4060
Merged
Merged
Changes from all commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
c18655f
ENH: use `dask.array.apply_gufunc` in `xr.apply_ufunc` for multiple o…
kmuehlbauer 3bf1d75
DOC: Update docstring and whats-new.rst
kmuehlbauer fff7660
WIP: apply_gufunc
kmuehlbauer d8bcb15
WIP: apply_gufunc -> reinstate dask='allowed' as per @mathause, adapt…
kmuehlbauer a17ca32
WIP: apply_gufunc -> add test for GH #4015, fix test for sparse meta …
kmuehlbauer 5f3f847
WIP: apply_gufunc -> remove unused `input_dims`
kmuehlbauer c9fd698
Update xarray/core/computation.py
kmuehlbauer fc30ee8
Update xarray/core/computation.py
kmuehlbauer 820a4cd
Update xarray/core/computation.py
kmuehlbauer 9a266dc
Merge remote-tracking branch 'origin/master' into fix-1815
kmuehlbauer 76375b4
WIP: use dask_gufunc_kwargs, keep vectorize first but only for non-da…
kmuehlbauer 6cff763
DOC: add reference to internal changes in whats-new.rst
kmuehlbauer eb953d2
FIX: mypy
kmuehlbauer 4ee835d
FIX: vectorize inside `apply_variable_ufunc`
kmuehlbauer ddeb1ea
TST: add tests from #4022 from @mathause
kmuehlbauer f645bbd
FIX: address black issue
kmuehlbauer c9e30af
FIX: xfail test for dask < 2.3
kmuehlbauer bdfdd74
Merge remote-tracking branch 'origin/master' into wip-apply-gufunc
kmuehlbauer f9cb53c
WIP: apply changes in response to @mathause's review comments
kmuehlbauer 3b652fb
WIP: remove line
kmuehlbauer 13f5c1d
WIP: catch different chunksize error and allow_rechunk, docstring fixes
kmuehlbauer 512d55d
WIP: remove comment
kmuehlbauer 5ad5063
WIP: style issues
kmuehlbauer 9f51c77
WIP: revert catch, revert test, add tests without output_dtypes
kmuehlbauer 41feeb3
Merge remote-tracking branch 'origin/master' into fix-1815
kmuehlbauer ada9cf0
WIP: fix signature in apply_ufunc->apply_gufunc, handle output_sizes,…
kmuehlbauer 1bdf2bb
WIP: fix tuple
kmuehlbauer 1401551
Merge remote-tracking branch 'origin/master' into fix-1815
kmuehlbauer fb64ed9
WIP: add dims_map to _UFuncSignature, adapt output_sizes to fit for a…
kmuehlbauer b0b5e2e
WIP: black
kmuehlbauer d0b4fb2
WIP: raise ValueError if output_sizes dimension mismatch
kmuehlbauer 1bca9de
WIP: raise ValueError if output_sizes is missing for given output_cor…
kmuehlbauer 1d5a8bb
WIP: simplify if/else
kmuehlbauer 91a9d5a
FIX: resolve conflicts prior merge with master
kmuehlbauer 035aa17
Merge remote-tracking branch 'origin/master' into fix-1815
kmuehlbauer e116fd0
FIX: combine if's as per review
kmuehlbauer 4db4444
FIX: pass `vectorize` and `output_dtypes` kwargs explicitely into `ap…
kmuehlbauer 9897c51
FIX: pass `vectorize` and `output_dtypes` kwargs explicitely into `da…
kmuehlbauer d4902a6
FIX: address review comments of @keewis and @mathause
kmuehlbauer 5a1f15e
FIX: black
kmuehlbauer a05bd18
FIX: `vectorize` not needed in if-clause
kmuehlbauer 35ae2a9
Merge remote-tracking branch 'origin/master' into fix-1815
kmuehlbauer 2fc6272
FIX: set DeprecationWarning and stacklevel=2
kmuehlbauer 4cb059e
FIX: use FutureWarning for user visibility
kmuehlbauer 4a48acd
FIX: remove comment as suggested
kmuehlbauer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Could you please set a class (DeprecationWarning) and stacklevel=2 on these warnings? That results in better messages for users.
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.
Sorry to nitpick - shouldn't that be a
FutureWarning
so that users actually get to see it?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.
@mathause At least in the tests the warnings are issued .
What's the actual difference between
DeprecationWarning
andFutureWarning
(update: just foundPendingDeprecationWarning
)? And when should they be used? Just to know for future contributions.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.
FutureWarning would be fine, too. We should probably try to come to consensus on a general policy for xarray.
The Python docs have some guidance but the overall recommendation is not really clear to me: https://docs.python.org/3/library/warnings.html#warning-categories
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.
FutureWarning
is for users andDeprecationWarning
for library authors (https://docs.python.org/3/library/warnings.html#warning-categories). Which is why you seeDeprecationWarning
in the test but won't when you execute the code. Took me a while to figure this out when I wanted to deprecate some stuff in my package.If you try this in ipython
test()
will raise both warnings. But if you save to a file and tryonly
FutureWarning
will appear (I did not know this detail either https://www.python.org/dev/peps/pep-0565/).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.
@mathause @shoyer I'll switch to
FutureWarning
since this seems to be the only user-visible warning, See https://www.python.org/dev/peps/pep-0565/#additional-use-case-for-futurewarningThere 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.
And, thanks for the pointers and explanations.