fill_value in shift #2470

max-sixty · 2018-10-06T20:33:29Z

Closes #Shift changes non-float arrays to object, even for shift=0 #2451
Tests added
Tests passed
Fully documented, including whats-new.rst for all changes and api.rst for 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)

Should we be more defensive around which fill_values can be passed? Currently, if the array and float values have incompatible dtypes, we don't preemtively warn or cast, apart from the case of np.nan, which then uses the default filler

pep8speaks · 2018-10-06T20:33:36Z

Hello @max-sixty! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 27, 2018 at 22:49 Hours UTC

max-sixty · 2018-10-07T18:18:19Z

xarray/core/dataarray.py

-        ds = self._to_temp_dataset().shift(shifts=shifts, **shifts_kwargs)
-        return self._from_temp_dataset(ds)
+        return self._replace(variable=self.variable.shift(
+            shifts=shifts, fill_value=fill_value, **shifts_kwargs))


Is this a reasonable change for DataArray operations that only change their data?

I think this is 100% equivalent -- I don't really care either way. But perhaps splitting this into two statements would be clearer.

shoyer

Looks great -- I have only a few minor points.

shoyer · 2018-10-07T22:33:48Z

xarray/core/dataarray.py

-        ds = self._to_temp_dataset().shift(shifts=shifts, **shifts_kwargs)
-        return self._from_temp_dataset(ds)
+        return self._replace(variable=self.variable.shift(
+            shifts=shifts, fill_value=fill_value, **shifts_kwargs))


I think this is 100% equivalent -- I don't really care either way. But perhaps splitting this into two statements would be clearer.

xarray/core/dataset.py

shoyer · 2018-10-07T22:37:45Z

xarray/core/variable.py

@@ -940,7 +940,11 @@ def _shift_one_dim(self, dim, count):
            keep = slice(None)

        trimmed_data = self[(slice(None),) * axis + (keep,)].data
-        dtype, fill_value = dtypes.maybe_promote(self.dtype)
+
+        if fill_value is None or fill_value is np.nan:


Use dtypes.NA as the default value for fill_value, and then copy these lines from pad to ensure that this works for arbitrary fill_values such as None:
https://github.com/pydata/xarray/blob/master/xarray/core/variable.py#L1012-L1015

I saw that, but it means that if someone passes np.nan (the value, not the dtypes.NA default) to a int dtype, I get -9223372036854775808. Should we raise in that case? Or I'll see what I can do to coerce.

Hmm. I'm pretty sure this would qualify as a NumPy bug :(.

I'm mostly concerned with duplicate logic that works slightly differently, so if you want to try doing this differently I'm open to it. It might actually make sense to replace most of this method to a call to Variable.pad_with_fill_value().

I'm mostly concerned with duplicate logic that works slightly differently

💯

Great, I'll have a look and report back

fujiisoup · 2018-12-24T12:48:02Z

xarray/core/variable.py

+        if fill_value is dtypes.NA and True:
+            dtype, fill_value = dtypes.maybe_promote(self.dtype)
+        else:
+            dtype = self.dtype


What happens if filler is not compatible with self.dtype?
For example, feeding np.nan to an int array.
Probably it is a part of user responsibility and we do not need to take care of this, but I am just curious of it.

In theory, NumPy should raise an error... But it may not.

(this is the issue I'm looking at ref #2470 (comment)), good foresight @fujiisoup !

shoyer

Looks good to me (except for the redundant clause in the if condition)

xarray/core/variable.py

shoyer · 2018-12-24T15:33:10Z

xarray/core/variable.py

+        if fill_value is dtypes.NA and True:
+            dtype, fill_value = dtypes.maybe_promote(self.dtype)
+        else:
+            dtype = self.dtype


In theory, NumPy should raise an error... But it may not.

max-sixty · 2018-12-27T19:12:33Z

I think this is in a decent place.

If np.nan is passed as fill_value into an int array, we'll get an odd result. But if they leave it to default, it'll work correctly, and I couldn't find a robust general way of solving that

Let me know thoughts!
Max

shoyer

Looks good to me (but the what's new note is now in the wrong place)

doc/whats-new.rst

* master: DEP: drop python 2 support and associated ci mods (pydata#2637) TST: silence warnings from bottleneck (pydata#2638) revert to dev version DOC: fix docstrings and doc build for 0.11.1 Source encoding always set when opening datasets (pydata#2626) Add flake check to travis (pydata#2632) Fix dayofweek and dayofyear attributes from dates generated by cftime_range (pydata#2633) silence import warning (pydata#2635) fill_value in shift (pydata#2470) Flake fixed (pydata#2629) Allow passing of positional arguments in `apply` for Groupby objects (pydata#2413) Fix failure in time encoding for pandas < 0.21.1 (pydata#2630) Fix multiindex selection (pydata#2621) Close files when CachingFileManager is garbage collected (pydata#2595) added some logic to deal with rasterio objects in addition to filepaths (pydata#2589) Get 0d slices of ndarrays directly from indexing (pydata#2625) FIX Don't raise a deprecation warning for xarray.ufuncs.{angle,iscomplex} (pydata#2615) CF: also decode time bounds when available (pydata#2571)

enable fill_value in shift

21ec3e4

max-sixty added 4 commits October 6, 2018 17:53

whatsnew

d79d132

docstrings

00e16ec

should we make some dataarray methods avoid rtp-ing to dataset?

404ed82

revert joining doc start

03587ac

max-sixty commented Oct 7, 2018

View reviewed changes

shoyer approved these changes Oct 7, 2018

View reviewed changes

max-sixty added 6 commits October 7, 2018 23:02

code comments

aaf1890

Merge branch 'master' into shift

6469b5a

WIP

b9a112c

Merge branch 'master' into shift

7feec29

Merge branch 'master' into shift

9a0ea70

Merge branch 'master' into shift

6ecf359

dcherian mentioned this pull request Oct 24, 2018

xarray 0.11 release #2505

Closed

5 tasks

max-sixty added 2 commits November 29, 2018 21:59

Merge branch 'master' into shift

048e287

Merge branch 'master' into shift

bb8ad25

fujiisoup reviewed Dec 24, 2018

View reviewed changes

shoyer approved these changes Dec 24, 2018

View reviewed changes

max-sixty added 3 commits December 25, 2018 15:04

Merge branch 'master' into shift

31a7048

pad use dict rather than kwargs

23f60b6

handle 'missing' values in a more consistent way in shift

1243a52

shoyer approved these changes Dec 27, 2018

View reviewed changes

doc/whats-new.rst Outdated Show resolved Hide resolved

doc/whats-new.rst Outdated Show resolved Hide resolved

whatsnew move

fccfe4d

shoyer merged commit 85ded91 into pydata:master Dec 27, 2018

max-sixty deleted the shift branch December 28, 2018 01:07

max-sixty mentioned this pull request Mar 4, 2019

Shift changes non-float arrays to object, even for shift=0 #2451

Closed

max-sixty mentioned this pull request Mar 5, 2020

Add DataArray.pad, Dataset.pad, Variable.pad #3596

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fill_value in shift #2470

fill_value in shift #2470

max-sixty commented Oct 6, 2018 •

edited

pep8speaks commented Oct 6, 2018 •

edited

max-sixty Oct 7, 2018

shoyer Oct 7, 2018

shoyer left a comment

shoyer Oct 7, 2018

shoyer Oct 7, 2018

max-sixty Oct 8, 2018

shoyer Oct 8, 2018

max-sixty Oct 8, 2018 •

edited

fujiisoup Dec 24, 2018

shoyer Dec 24, 2018

max-sixty Dec 27, 2018

shoyer left a comment

shoyer Dec 24, 2018

max-sixty commented Dec 27, 2018

shoyer left a comment

fill_value in shift #2470

fill_value in shift #2470

Conversation

max-sixty commented Oct 6, 2018 • edited

pep8speaks commented Oct 6, 2018 • edited

Comment last updated on December 27, 2018 at 22:49 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-sixty Oct 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-sixty commented Dec 27, 2018

shoyer left a comment

Choose a reason for hiding this comment

max-sixty commented Oct 6, 2018 •

edited

pep8speaks commented Oct 6, 2018 •

edited

max-sixty Oct 8, 2018 •

edited