implemented pad with new-indexes #4974

fujiisoup · 2021-03-01T07:50:08Z

Closes What should pad do about IndexVariables? #3868
Tests added
Passes pre-commit run --all-files
User visible changes (including notable bug fixes) are documented in whats-new.rst

Now we use a tuple of indexes for DataArray.pad and Dataset.pad.

pep8speaks · 2021-03-01T07:50:20Z

Hello @fujiisoup! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file xarray/core/dataarray.py:

Line 3954:1: W293 blank line contains whitespace

Comment last updated at 2021-06-29 11:56:41 UTC

fujiisoup · 2021-03-01T08:00:42Z

xarray/core/dataset.py

@@ -6638,8 +6653,23 @@ def pad(
            coord_pad_options = {}

        variables = {}
+
+        # standarize pad_width
+        pad_width_standarized = {}  # type: Mapping[Hashable, Tuple[int, int]]


Needs help for correcting the typing.
Now, pad_width_standarized should be Mapping[Hashable, Tuple[int, int]] instead of Mapping[ Hashable, Union[int, Tuple[Union[int, Iterable], Union[int, Iterable]]] ]
mypy is complaining...

mathause

That's not trivial to correctly type... The main change I did was to use Sequence (https://docs.python.org/3/library/collections.abc.html) as Iterable has no __len__. E.g. numpy uses Sequence for ArrayLike (https://github.com/numpy/numpy/blob/master/numpy/typing/_array_like.py).

Disclaimer: I am by no means a specialist here - I am making this up as I go.

xarray/core/dataarray.py

xarray/core/dataset.py

mathause · 2021-03-01T18:46:37Z

xarray/core/dataset.py

+            if not isinstance(v, int):
+                # if pad_width is a tuple of iterable, we use its length for
+                # pad_width_standarized
+                pad_width_standarized[k] = [


Suggested change

pad_width_standarized[k] = [

pad_width_standardized[k] = [

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

xarray/core/dataset.py

mathause · 2021-03-02T14:20:59Z

I fixed the typo (pad_width_standardized) but I haven't really reviewed the code... There is a doctest that fails - da.pad(dim=1) that should be added as test & fixed.

doc/whats-new.rst

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com> Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

fujiisoup · 2021-03-03T07:44:04Z

Not sure why the doctest is failing. The same tests in test_dataset.py do not fail...

mathause · 2021-03-03T09:36:22Z

I think only the indentation is wrong now - e.g. https://github.com/pydata/xarray/pull/4974/checks?check_run_id=2020140730#step:8:82

fujiisoup · 2021-03-03T20:08:18Z

Thank you @mathause for your suggestion.
This looks all the tests are passing now.

fujiisoup · 2021-03-03T20:09:59Z

xarray/core/dataset.py

+                    w1_ = IndexVariable(name, [fill_value_ind] * w1)
+                else:
+                    w1_ = IndexVariable(name, w1)
+                variables[name] = var.concat([w0_, var, w1_], dim=name)


I'm a bit wondering if we put this series of logic should be moved into IndexVariable.pad.

Yes I think that would make sense.

Thanks. Done.

disallow [Sequence, int] as an argument.

fujiisoup · 2021-03-05T21:38:01Z

xarray/core/dataset.py

+                if isinstance(pad_start, int) or isinstance(pad_end, int):
+                    # do not allow [Sequence, int] as pad_width
+                    raise TypeError(
+                        "({}, {}) is used for pad_width[{}]. Must be either (int, int) or (Sequence, Sequence).".format(


Noticed that a mixture of sequence and int for pad_width, like

da.pad(dim2=([0, 1, 2], 3))

makes implementation very complex (as we may need to support many options, such as pad_mode).
I just disallowed the mixed argument.

xarray/core/dataarray.py

dcherian · 2021-03-09T16:44:44Z

xarray/core/variable.py

@@ -1355,6 +1355,15 @@ def pad(

        return type(self)(self.dims, array)

+    def pad_indexes(self, pad_start: Sequence, pad_end: Sequence):


Make this IndexVariable.pad? and add a test in test_variable.py?

xarray/tests/test_dataset.py

dcherian · 2021-03-09T16:48:15Z

xarray/core/dataset.py

+                if isinstance(pad_start, int) or isinstance(pad_end, int):
+                    # do not allow [Sequence, int] as pad_width
+                    raise TypeError(
+                        "({}, {}) is used for pad_width[{}]. Must be either (int, int) or (Sequence, Sequence).".format(


xarray/tests/test_dataset.py

benbovy · 2022-02-22T16:47:01Z

I was wondering what is the status of this PR?

@fujiisoup do you still plan to work on it? Or maybe is it wise to wait for the indexes refactoring (#5692), i.e., #3868 (comment) and revisit a bit the approach?

More specifically, the approach here (i.e., passing indexes via the pad_width argument) may be tricky in the context of flexible indexes where multiple indexes/coordinates are allowed for one dimension.

What about another parameter data_values to explicitly pass values to certain (coordinate) variables? It would accept something like {var_name: (before_values, after_values)} and would be passed to an Index.pad new method (if var_name is a coordinate with an index) or to Variable.pad as a fallback. This would keep pad_width simple. We'd just need to check that the arrays passed in data_values have dimensions matching with the dimensions of the corresponding variables and have sizes or shapes matching with the sizes specified in pad_width.

fujiisoup · 2022-02-24T03:02:43Z

Hi.
Sorry for my late reply.
Well, I've just left this PR untouched.

More specifically, the approach here (i.e., passing indexes via the pad_width argument) may be tricky in the context of flexible indexes where multiple indexes/coordinates are allowed for one dimension.

I think we can just discard this PR if this does not fit with the index refactoring.
This PR is not big anyway and maybe rewriting this functionality is faster.

jhamman · 2023-09-14T02:47:24Z

Closing this per the last comments and label added to this PR last year.

implemented pad with new-indexes

b8cc48b

pre-commit hook

f65c4c9

fujiisoup commented Mar 1, 2021

View reviewed changes

mathause reviewed Mar 1, 2021

View reviewed changes

fujiisoup and others added 3 commits March 2, 2021 17:01

Update xarray/core/dataset.py

4a4c514

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

Update xarray/core/dataarray.py

f40ae1d

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

Apply suggestions from code review

ed7cfa2

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

mathause reviewed Mar 2, 2021

View reviewed changes

Apply suggestions from code review

36e3e39

dcherian reviewed Mar 2, 2021

View reviewed changes

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

fujiisoup and others added 4 commits March 3, 2021 16:13

Apply suggestions from code review

c49ef3a

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com> Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

Update xarray/core/dataset.py

99daf64

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

A bug fix; now int can be passed as an argument.

07a01b4

Added equivalent tests for doctest, but not failing

6662385

fujiisoup added 2 commits March 4, 2021 04:09

Fix indent

c130af4

doctest

ac7bf70

fujiisoup commented Mar 3, 2021

View reviewed changes

fujiisoup added 2 commits March 6, 2021 06:34

Added Variable.pad_indexes

9b90769

disallow [Sequence, int] as an argument.

Merge branch 'master' into index_pad

69abc13

fujiisoup commented Mar 5, 2021

View reviewed changes

Merge branch 'master' into index_pad

1ae6afb

dcherian reviewed Mar 9, 2021

View reviewed changes

Update xarray/core/dataarray.py

30391c6

santisoler mentioned this pull request Jul 1, 2021

Add pad and unpad functions xgcm/xrft#158

Merged

2 tasks

headtr1ck added topic-indexing plan to close May be closeable, needs more eyeballs labels Nov 21, 2022

jhamman closed this Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented pad with new-indexes #4974

implemented pad with new-indexes #4974

fujiisoup commented Mar 1, 2021 •

edited by keewis

Loading

pep8speaks commented Mar 1, 2021 •

edited

Loading

fujiisoup Mar 1, 2021

mathause left a comment

mathause Mar 1, 2021

mathause commented Mar 2, 2021

fujiisoup commented Mar 3, 2021

mathause commented Mar 3, 2021

fujiisoup commented Mar 3, 2021

fujiisoup Mar 3, 2021

mathause Mar 4, 2021

fujiisoup Mar 5, 2021

fujiisoup Mar 5, 2021

dcherian Mar 9, 2021

dcherian Mar 9, 2021 •

edited

Loading

dcherian Mar 9, 2021

benbovy commented Feb 22, 2022

fujiisoup commented Feb 24, 2022

jhamman commented Sep 14, 2023

		@@ -1355,6 +1355,15 @@ def pad(

		return type(self)(self.dims, array)

		def pad_indexes(self, pad_start: Sequence, pad_end: Sequence):

implemented pad with new-indexes #4974

implemented pad with new-indexes #4974

Conversation

fujiisoup commented Mar 1, 2021 • edited by keewis Loading

pep8speaks commented Mar 1, 2021 • edited Loading

Comment last updated at 2021-06-29 11:56:41 UTC

Choose a reason for hiding this comment

mathause left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathause commented Mar 2, 2021

fujiisoup commented Mar 3, 2021

mathause commented Mar 3, 2021

fujiisoup commented Mar 3, 2021

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

dcherian Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbovy commented Feb 22, 2022

fujiisoup commented Feb 24, 2022

jhamman commented Sep 14, 2023

fujiisoup commented Mar 1, 2021 •

edited by keewis

Loading

pep8speaks commented Mar 1, 2021 •

edited

Loading

dcherian Mar 9, 2021 •

edited

Loading