Deprecate inplace #2524
Deprecate inplace #2524
Conversation
|
Hello @dcherian! Thanks for submitting the PR.
|
xarray/core/dataset.py
Outdated
| @@ -2494,12 +2501,13 @@ def update(self, other, inplace=True): | |||
| If any dimensions would have inconsistent sizes in the updated | |||
| dataset. | |||
| """ | |||
| # _check_inplace(inplace) | |||
There was a problem hiding this comment.
@shoyer What's the right thing to do here? Changing the default to None/False breaks a lot of test.
There was a problem hiding this comment.
Dataset.update() has always been an in-place operation for consistency with dict.update(). I don't think it really makes sense to change that.
I would suggest that we do the deprecation the other direction here: remove the inplace argument but keep the behavior as always inplace.
xarray/core/utils.py
Outdated
| inplace = default | ||
| else: | ||
| warnings.warn('The inplace argument has been deprecated and will be ' | ||
| 'removed in xarray 0.12.0.', FutureWarning) |
There was a problem hiding this comment.
Can you add the stacklevel argument with an appropriate value? (you'll need to experiment a bit)
That ensures that warnings end up pointing to the specific line of code that need to be updated.
There was a problem hiding this comment.
stacklevel=3 seems to give useful warnings with the test suite
xarray/tests/test_dataset.py::TestDataset::()::test_expand_dims_error
/home/deepak/work/python/xarray/xarray/tests/test_dataset.py:2016: FutureWarning: The inplace argument has been deprecated and will be removed in xarray 0.12.0.
original.set_coords('z', inplace=True)
* master: Global option to always keep/discard attrs on operations (pydata#2482) Remove tests where answers change in cftime 1.0.2.1 (pydata#2522) Finish deprecation cycle for DataArray.__contains__ checking array values (pydata#2520) Fix bug where OverflowError is not being raised (pydata#2519)
|
Failed test is dask distributed test |
xarray/tests/test_dataarray.py
Outdated
| @@ -23,6 +23,7 @@ | |||
| requires_scipy, source_ndarray) | |||
|
|
|||
|
|
|||
| @pytest.mark.filterwarnings('ignore:The inplace argument') | |||
There was a problem hiding this comment.
Can we try fixing these tests now instead? Otherwise we may be stuck with large clean-up later.
For testing deprecations, I find it often helps to convert warnings into an error, e.g., by adding raise Exception on the line after issuing the warning.
There was a problem hiding this comment.
I thought we might want to keep those tests around while people are still using that code path.
There was a problem hiding this comment.
I forgot the main reason for doing this: to make sure that xarray doesn't rely upon the deprecated behavior internally.
But if you already checked that, then I suppose this is fine.
There was a problem hiding this comment.
Oh I haven't checked that closely but I can do that tonight.
(my impression was that the tests were testing inplace behaviour, but I see that could be wrong)
|
In general, I would be OK with *not* testing inplace behavior carefully. We
can certainly add this to specific test methods but overall I think it's
better to have slightly less test coverage of deprecated behavior rather
than to raise deprecation warnings as part of unrelated operations.
…On Tue, Oct 30, 2018 at 3:06 PM Deepak Cherian ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In xarray/tests/test_dataarray.py
<#2524 (comment)>:
> @@ -23,6 +23,7 @@
requires_scipy, source_ndarray)
***@***.***('ignore:The inplace argument')
Oh I haven't checked that closely but I can do that tonight.
(my impression was that the tests were testing inplace behaviour, but I
see that could be wrong)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2524 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1nCL0fqW2POPBepyKA2R4lxQGGONks5uqM1ZgaJpZM4X-bI->
.
|
|
OK so what's left is |
|
_calculate_binary_op shouldn't lose the inplace argument -- it's used for
Python inplace arithmetic like +=.
…On Wed, Oct 31, 2018 at 12:00 PM Deepak Cherian ***@***.***> wrote:
OK so what's left is Dataset._calculate_binary_op(g, other, inplace=True)
called by Dataset._inplace_binary_op at Line 3307 in dataset.py. I don't
know what the right fix is.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2524 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1m-T_2r1AXYZxnQtFr3NNXEPoNgeks5uqfNEgaJpZM4X-bI->
.
|
|
thanks @dcherian |
* upstream/master: (122 commits) add missing , and article in error message (pydata#2557) Add libnetcdf, libhdf5, pydap and cfgrib to xarray.show_versions() (pydata#2555) revert to dev version for 0.11.1 Release xarray v0.11 DOC: update whatsnew for xarray 0.11 release (pydata#2548) Drop the hack needed to use CachingFileManager as we don't use it anymore. (pydata#2544) add full test env for py37 ci env (pydata#2545) Remove old-style resample example in documentation (pydata#2543) Stop loading tutorial data by default (pydata#2538) Remove the old syntax for resample. (pydata#2541) Remove use of deprecated, unused keyword. (pydata#2540) Deprecate inplace (pydata#2524) Zarr chunking (GH2300) (pydata#2487) Include multidimensional stacking groupby in docs (pydata#2493) (pydata#2536) Switch enable_cftimeindex to True by default (pydata#2516) Raise more informative error when converting tuples to Variable. (pydata#2523) Global option to always keep/discard attrs on operations (pydata#2482) Remove tests where answers change in cftime 1.0.2.1 (pydata#2522) Finish deprecation cycle for DataArray.__contains__ checking array values (pydata#2520) Fix bug where OverflowError is not being raised (pydata#2519) ...
whats-new.rstfor all changes andapi.rstfor new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)