Fixing deepcopy for Dataset.attrs #2839
Conversation
|
Hello @kefirbandi! 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 2019-03-25 02:33:28 UTC |
| @@ -1905,6 +1906,8 @@ def test_copy(self): | |||
| v1 = copied.variables[k] | |||
| assert v0 is not v1 | |||
|
|
|||
| assert data.attrs['Test'] is not copied.attrs['Test'] | |||
There was a problem hiding this comment.
can you also add a test for shallow copying?
xarray/core/dataset.py
Outdated
| @@ -709,6 +709,7 @@ def _replace( # type: ignore | |||
| indexes: 'Optional[OrderedDict[Any, pd.Index]]' = __default, | |||
| encoding: Optional[dict] = __default, | |||
| inplace: bool = False, | |||
| deep: bool = False, | |||
There was a problem hiding this comment.
Rather than modifying _replace, could you use pass in attrs explicitly from Dataset.copy() itself?
I'd like to keep this method relatively simple -- it already has a large number of keyword arguments.
Removing additional argument of Dataset._replace
| @@ -915,7 +915,9 @@ def copy(self: T, deep: bool = False, data: Mapping = None) -> T: | |||
| variables = OrderedDict((k, v.copy(deep=deep, data=data.get(k))) | |||
| for k, v in self._variables.items()) | |||
|
|
|||
| return self._replace(variables) | |||
| attrs = copy.deepcopy(self._attrs) if deep else copy.copy(self._attrs) | |||
There was a problem hiding this comment.
I made one small fix, renaming _attrs to attrs.
As a general rule, we use the underscore prefix in Python as a convention for "private" but technically public variables. This isn't necessary for local variables defined within the scope of a function, since these are already private.
There was a problem hiding this comment.
Right. Actually I first called it "new_attrs" but it was 3 characters above PEP8 line-length limit, so I removed the first 3... :)
Thanks for the fix.
whats-new.rstfor all changes andapi.rstfor new API