Skip to content

Deprecate inplace #2524

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.

Sign up for GitHub

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

Merged
shoyer merged 10 commits into from
Nov 3, 2018
Merged

Deprecate inplace #2524

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
10fc573
Start deprecating inplace.
Oct 29, 2018
edf1a0a
remove warnings from tests.
Oct 29, 2018
3a5cf5d
this commit silences nearly all warnings.
Oct 29, 2018
b1ed4df
Add whats-new.
Oct 29, 2018
484d71f
Add a default kwarg to _check_inplace and use for Dataset.update.
Oct 29, 2018
63d4175
Major fix!
Oct 29, 2018
66d3cea
Add stacklevel
Oct 30, 2018
4359403
Merge branch 'master' into deprecate/inplace
Oct 30, 2018
62faebc
Tests: Less aggressive warning filter + fix unnecessary inplace.
Oct 31, 2018
7a0d37b
revert changes to _calculate_binary_op
Nov 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whats-new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ Documentation
without dimension argument will change in the next release.
Now we warn a FutureWarning.
By `Keisuke Fujii <https://github.com/fujiisoup>`_.
- The ``inplace`` kwarg of a number of `DataArray` and `Dataset` methods is being
deprecated and will be removed in the next release.
By `Deepak Cherian <https://github.com/dcherian>`_.

Enhancements
~~~~~~~~~~~~
Expand Down
15 changes: 10 additions & 5 deletions xarray/core/dataarray.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
from .options import OPTIONS
from .pycompat import OrderedDict, basestring, iteritems, range, zip
from .utils import (
decode_numpy_dict_values, either_dict_or_kwargs, ensure_us_time_resolution)
_check_inplace, decode_numpy_dict_values, either_dict_or_kwargs,
ensure_us_time_resolution)
from .variable import (
IndexVariable, Variable, as_compatible_data, as_variable,
assert_unique_multiindex_level_names)
Expand Down Expand Up @@ -546,7 +547,7 @@ def coords(self):
"""
return DataArrayCoordinates(self)

def reset_coords(self, names=None, drop=False, inplace=False):
def reset_coords(self, names=None, drop=False, inplace=None):
"""Given names of coordinates, reset them to become variables.

Parameters
Expand All @@ -565,6 +566,7 @@ def reset_coords(self, names=None, drop=False, inplace=False):
-------
Dataset, or DataArray if ``drop == True``
"""
_check_inplace(inplace)
if inplace and not drop:
raise ValueError('cannot reset coordinates in-place on a '
'DataArray without ``drop == True``')
Expand Down Expand Up @@ -1155,7 +1157,7 @@ def expand_dims(self, dim, axis=None):
ds = self._to_temp_dataset().expand_dims(dim, axis)
return self._from_temp_dataset(ds)

def set_index(self, indexes=None, append=False, inplace=False,
def set_index(self, indexes=None, append=False, inplace=None,
**indexes_kwargs):
"""Set DataArray (multi-)indexes using one or more existing
coordinates.
Expand Down Expand Up @@ -1185,14 +1187,15 @@ def set_index(self, indexes=None, append=False, inplace=False,
--------
DataArray.reset_index
"""
_check_inplace(inplace)
indexes = either_dict_or_kwargs(indexes, indexes_kwargs, 'set_index')
coords, _ = merge_indexes(indexes, self._coords, set(), append=append)
if inplace:
self._coords = coords
else:
return self._replace(coords=coords)

def reset_index(self, dims_or_levels, drop=False, inplace=False):
def reset_index(self, dims_or_levels, drop=False, inplace=None):
"""Reset the specified index(es) or multi-index level(s).

Parameters
Expand All @@ -1217,14 +1220,15 @@ def reset_index(self, dims_or_levels, drop=False, inplace=False):
--------
DataArray.set_index
"""
_check_inplace(inplace)
coords, _ = split_indexes(dims_or_levels, self._coords, set(),
self._level_coords, drop=drop)
if inplace:
self._coords = coords
else:
return self._replace(coords=coords)

def reorder_levels(self, dim_order=None, inplace=False,
def reorder_levels(self, dim_order=None, inplace=None,
**dim_order_kwargs):
"""Rearrange index levels using input order.

Expand All @@ -1247,6 +1251,7 @@ def reorder_levels(self, dim_order=None, inplace=False,
Another dataarray, with this dataarray's data but replaced
coordinates.
"""
_check_inplace(inplace)
dim_order = either_dict_or_kwargs(dim_order, dim_order_kwargs,
'reorder_levels')
replace_coords = {}
Expand Down
35 changes: 22 additions & 13 deletions xarray/core/dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@
from .pycompat import (
OrderedDict, basestring, dask_array_type, integer_types, iteritems, range)
from .utils import (
Frozen, SortedKeysDict, datetime_to_numeric, decode_numpy_dict_values,
either_dict_or_kwargs, ensure_us_time_resolution, hashable,
maybe_wrap_array)
_check_inplace, Frozen, SortedKeysDict, datetime_to_numeric,
decode_numpy_dict_values, either_dict_or_kwargs, ensure_us_time_resolution,
hashable, maybe_wrap_array)
from .variable import IndexVariable, Variable, as_variable, broadcast_variables

# list of attributes of pd.DatetimeIndex that are ndarrays of time info
Expand Down Expand Up @@ -1072,7 +1072,7 @@ def data_vars(self):
"""
return DataVariables(self)

def set_coords(self, names, inplace=False):
def set_coords(self, names, inplace=None):
"""Given names of one or more variables, set them as coordinates

Parameters
Expand All @@ -1095,14 +1095,15 @@ def set_coords(self, names, inplace=False):
# DataFrame.set_index?
# nb. check in self._variables, not self.data_vars to insure that the
# operation is idempotent
_check_inplace(inplace)
if isinstance(names, basestring):
names = [names]
self._assert_all_in_dataset(names)
obj = self if inplace else self.copy()
obj._coord_names.update(names)
return obj

def reset_coords(self, names=None, drop=False, inplace=False):
def reset_coords(self, names=None, drop=False, inplace=None):
"""Given names of coordinates, reset them to become variables

Parameters
Expand All @@ -1121,6 +1122,7 @@ def reset_coords(self, names=None, drop=False, inplace=False):
-------
Dataset
"""
_check_inplace(inplace)
if names is None:
names = self._coord_names - set(self.dims)
else:
Expand Down Expand Up @@ -2045,7 +2047,7 @@ def interp_like(self, other, method='linear', assume_sorted=False,
ds = self.reindex(object_coords)
return ds.interp(numeric_coords, method, assume_sorted, kwargs)

def rename(self, name_dict=None, inplace=False, **names):
def rename(self, name_dict=None, inplace=None, **names):
"""Returns a new object with renamed variables and dimensions.

Parameters
Expand All @@ -2070,6 +2072,7 @@ def rename(self, name_dict=None, inplace=False, **names):
Dataset.swap_dims
DataArray.rename
"""
_check_inplace(inplace)
name_dict = either_dict_or_kwargs(name_dict, names, 'rename')
for k, v in name_dict.items():
if k not in self and k not in self.dims:
Expand All @@ -2095,7 +2098,7 @@ def rename(self, name_dict=None, inplace=False, **names):
return self._replace_vars_and_dims(variables, coord_names, dims=dims,
inplace=inplace)

def swap_dims(self, dims_dict, inplace=False):
def swap_dims(self, dims_dict, inplace=None):
"""Returns a new object with swapped dimensions.

Parameters
Expand All @@ -2119,6 +2122,7 @@ def swap_dims(self, dims_dict, inplace=False):
Dataset.rename
DataArray.swap_dims
"""
_check_inplace(inplace)
for k, v in dims_dict.items():
if k not in self.dims:
raise ValueError('cannot swap from dimension %r because it is '
Expand Down Expand Up @@ -2231,7 +2235,7 @@ def expand_dims(self, dim, axis=None):

return self._replace_vars_and_dims(variables, self._coord_names)

def set_index(self, indexes=None, append=False, inplace=False,
def set_index(self, indexes=None, append=False, inplace=None,
**indexes_kwargs):
"""Set Dataset (multi-)indexes using one or more existing coordinates or
variables.
Expand Down Expand Up @@ -2262,14 +2266,15 @@ def set_index(self, indexes=None, append=False, inplace=False,
Dataset.reset_index
Dataset.swap_dims
"""
_check_inplace(inplace)
indexes = either_dict_or_kwargs(indexes, indexes_kwargs, 'set_index')
variables, coord_names = merge_indexes(indexes, self._variables,
self._coord_names,
append=append)
return self._replace_vars_and_dims(variables, coord_names=coord_names,
inplace=inplace)

def reset_index(self, dims_or_levels, drop=False, inplace=False):
def reset_index(self, dims_or_levels, drop=False, inplace=None):
"""Reset the specified index(es) or multi-index level(s).

Parameters
Expand All @@ -2293,13 +2298,14 @@ def reset_index(self, dims_or_levels, drop=False, inplace=False):
--------
Dataset.set_index
"""
_check_inplace(inplace)
variables, coord_names = split_indexes(dims_or_levels, self._variables,
self._coord_names,
self._level_coords, drop=drop)
return self._replace_vars_and_dims(variables, coord_names=coord_names,
inplace=inplace)

def reorder_levels(self, dim_order=None, inplace=False,
def reorder_levels(self, dim_order=None, inplace=None,
**dim_order_kwargs):
"""Rearrange index levels using input order.

Expand All @@ -2322,6 +2328,7 @@ def reorder_levels(self, dim_order=None, inplace=False,
Another dataset, with this dataset's data but replaced
coordinates.
"""
_check_inplace(inplace)
dim_order = either_dict_or_kwargs(dim_order, dim_order_kwargs,
'reorder_levels')
replace_variables = {}
Expand Down Expand Up @@ -2494,12 +2501,13 @@ def update(self, other, inplace=True):
If any dimensions would have inconsistent sizes in the updated
dataset.
"""
# _check_inplace(inplace)
Copy link
Contributor Author

@dcherian dcherian Oct 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shoyer What's the right thing to do here? Changing the default to None/False breaks a lot of test.

Copy link
Member

@shoyer shoyer Oct 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

variables, coord_names, dims = dataset_update_method(self, other)

return self._replace_vars_and_dims(variables, coord_names, dims,
inplace=inplace)

def merge(self, other, inplace=False, overwrite_vars=frozenset(),
def merge(self, other, inplace=None, overwrite_vars=frozenset(),
compat='no_conflicts', join='outer'):
"""Merge the arrays of two datasets into a single dataset.

Expand Down Expand Up @@ -2550,6 +2558,7 @@ def merge(self, other, inplace=False, overwrite_vars=frozenset(),
MergeError
If any variables conflict (see ``compat``).
"""
_check_inplace(inplace)
variables, coord_names, dims = dataset_merge_method(
self, other, overwrite_vars=overwrite_vars, compat=compat,
join=join)
Expand Down Expand Up @@ -3311,8 +3320,8 @@ def func(self, other):
return func

def _calculate_binary_op(self, f, other, join='inner',
inplace=False):

inplace=None):
_check_inplace(inplace)
def apply_over_both(lhs_data_vars, rhs_data_vars, lhs_vars, rhs_vars):
if inplace and set(lhs_data_vars) != set(rhs_data_vars):
raise ValueError('datasets must have the same data variables '
Expand Down
8 changes: 8 additions & 0 deletions xarray/core/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@
OrderedDict, basestring, bytes_type, dask_array_type, iteritems)


def _check_inplace(inplace):
if inplace is None:
inplace = False
else:
warnings.warn('The inplace argument has been deprecated and will be '
'removed in xarray 0.12.0.', FutureWarning)
Copy link
Member

@shoyer shoyer Oct 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dcherian dcherian Oct 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)



def alias_message(old_name, new_name):
return '%s has been deprecated. Use %s instead.' % (old_name, new_name)

Expand Down
1 change: 1 addition & 0 deletions xarray/tests/test_dataarray.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
requires_scipy, source_ndarray)


@pytest.mark.filterwarnings('ignore:The inplace argument')
Copy link
Member

@shoyer shoyer Oct 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dcherian dcherian Oct 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we might want to keep those tests around while people are still using that code path.

Copy link
Member

@shoyer shoyer Oct 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dcherian dcherian Oct 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

class TestDataArray(object):
@pytest.fixture(autouse=True)
def setup(self):
Expand Down
1 change: 1 addition & 0 deletions xarray/tests/test_dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def lazy_inaccessible(k, v):
k, v in iteritems(self._variables))


@pytest.mark.filterwarnings('ignore:The inplace argument')
class TestDataset(object):
def test_repr(self):
data = create_test_data(seed=123)
Expand Down
2 changes: 1 addition & 1 deletion xarray/tests/test_plot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,7 @@ def setUp(self):
x, y = np.meshgrid(da.x.values, da.y.values)
ds['x2d'] = DataArray(x, dims=['y', 'x'])
ds['y2d'] = DataArray(y, dims=['y', 'x'])
ds.set_coords(['x2d', 'y2d'], inplace=True)
ds = ds.set_coords(['x2d', 'y2d'])
# set darray and plot method
self.darray = ds.testvar

Expand Down