FIX: fix interpolate with kwarg limit area and limit direction using pad or bfill #31048

cchwala · 2020-01-15T17:06:53Z

closes limit_area and limit_direction do not have an effect when interpolation method is 'pad' #26796
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

This separates a large part of the code changes from #25141 to keep things comprehendible.

This test currently only test `limit_area`. For `limit_direction` the implementation should later raise an error, because `pad` and `bfill` both already define a direction. But let's now first do the implementation of the `limit_area` for `pad` and `bfill`.

Since methods `pad` and `bfill` in `blocks.interpolate` end up using `missing.interpolate_2d` which can not (easily) be extended to support `limit_area`, I introduce the new function `missing.interpolate_1d_fill`. It is a modified copy of `interpolate_2d` but only works for 1d data and uses newly introduced function `_derive_indices_of_nans_to_preserve`, which is now also used in `missing.interpolate_1d`. It works the same way as the 1D-interpolation functions which are based on scipy-interpolation which are applied via np.apply_along_axis.

pep8speaks · 2020-01-15T17:07:00Z

Hello @cchwala! 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 2020-09-15 15:40:12 UTC

WillAyd · 2020-01-15T18:39:52Z

@cchwala so IIUC you are doing a precursor to solve the issue with pad and limit_area and then planning to add max_gap after that gets merged in the original PR right?

cchwala · 2020-01-15T20:37:35Z

@WillAyd Yes. That is my plan.

I am reusing the code from the PR with max_gap that actually solves #26796. The max_gap PR will be a lot smaller when the changes from this PR are merge.

…values` also was changed via appliying `func`

…e used Test for all forbidden combos of `pad` and `backfill` is included

…d `limit_direction`

* black formatting * typo

WillAyd

Looks pretty good. If you can annotate any new functions would be helpful. Added what I think should be

WillAyd · 2020-01-15T21:44:29Z

pandas/core/missing.py

@@ -477,6 +496,65 @@ def _akima_interpolate(xi, yi, x, der=0, axis=0):
    else:
        return [P(x, nu) for nu in der]

+def interpolate_1d_fill(
+    values,
+    method="pad",


Suggested change

method="pad",

method: str = "pad",

WillAyd · 2020-01-15T21:44:43Z

pandas/core/missing.py

+def interpolate_1d_fill(
+    values,
+    method="pad",
+    axis=0,


Suggested change

axis=0,

axis: Axis = 0,

(import from pandas._typing)

WillAyd · 2020-01-15T21:45:33Z

pandas/core/missing.py

+    values,
+    method="pad",
+    axis=0,
+    limit=None,


Suggested change

limit=None,

limit: Optional[int] = None,

WillAyd · 2020-01-15T21:45:44Z

pandas/core/missing.py

+    method="pad",
+    axis=0,
+    limit=None,
+    limit_area=None,


Suggested change

limit_area=None,

limit_area: Optional[str] = None,

WillAyd · 2020-01-15T21:46:03Z

pandas/core/missing.py

+    limit=None,
+    limit_area=None,
+    fill_value=None,
+    dtype=None,


Suggested change

dtype=None,

dtype: Optional[Dtype] = None,

(import from pandas._typing)

WillAyd · 2020-01-15T21:46:14Z

pandas/core/missing.py

+    axis=0,
+    limit=None,
+    limit_area=None,
+    fill_value=None,


Suggested change

fill_value=None,

fill_value: Optional[Hashable] = None,

cchwala · 2020-01-15T22:50:45Z

Should be ready now. I am offline for some hours of sleep, though.

cchwala · 2020-01-16T16:29:12Z

@WillAyd Any further requests for changes?

pandas/core/generic.py

jreback

looks pretty good

pandas/core/missing.py

pandas/core/internals/blocks.py

jreback · 2020-01-18T17:30:04Z

pandas/core/generic.py

@@ -6720,6 +6722,28 @@ def interpolate(
                "column to a numeric dtype."
            )

+        # Set `limit_direction` depending on `method`


is there a reason this logic does not simply belong in interpolate_2d?

or call something like clean_fill_method

limit_direction has no effect in interpolate_2d.

My idea was to detect conflicting user input, e.g. pad with limit_direction='backward' as early as possible in the chain of calls to the many functions that are involved.

But I can move the logic to a function like clean_fill_method, e.g. called set_limit_direction. Should this function belong to generic.py or missing.py?

interpolate_2d is also called from fillna. only NDFrame.interpolate allows limit_direction so does make sense to validate here.

clean_fill_method checks method. probably want to avoid passing more parameters around.

pls make a method similr to clean_fill_method then, e.g this logic should living pandas/core/missing.py

not sure what to do here. after merge with master, there is similar code at this location, but it is not mine...

jreback · 2020-01-18T17:31:37Z

pandas/core/missing.py

@@ -478,6 +500,66 @@ def _akima_interpolate(xi, yi, x, der=0, axis=0):
        return [P(x, nu) for nu in der]


+def interpolate_1d_fill(


can you move this under interpolate_1d

interpolate_1d_fill is a special case needed by blocks._interpolate_with_fill() to support the limit kwargs with method pad. As you requested in the comment above interpolate_1d_fill is now called from within interpolate_2d which is what blocks._interpolate_with_fill() calls.

You request, moving it to missing.interpolate_1d will, in my opinion, not work, because missing.interpolate_1d is not reached in the call sequence for interpolating with method pad since blocks.Block.interpolate() splits up into blocks.Block._interpolate (for scipy wrappers) and blocks.Block._interpolate_with_fill (for pad methods). missing.interpolate_1d is only called in the former case, i.e. for methods using the scipy wrappers.

@cchwala interpolate_2d will work on a 1d array. did you investigate applying it along an axis (with masking logic) rather than creating a 1d version.

see #34749 for alternative implementation calling interpolate_2d instead. interpolate_2d already has the limit logic so no need to use the preserve_nans set based logic.

I used the nice solution from #34749 as suggested above

as a consequence there is no interpolate_1d_fill anymore

doc/source/whatsnew/v1.0.0.rst

…ments Test on my local machine are not affected by removing the unncessery arguments `valid` and `invalid`, which are now derived within the function.

WillAyd · 2020-02-12T00:54:00Z

@cchwala can you address comments?

…erpolate_2d`

simonjayhawkins · 2020-08-25T14:15:00Z

not really sure of the status here and @simonjayhawkins had some cleanups as well, can we clarify.

I did break some bits off this PR. so does need a merge of master to see what's left (I did try this #31048 (comment))

cchwala · 2020-08-25T15:27:43Z

Okay. I will do the merge with master today to see what is left. I guess, at least the tests will still be of value.

…imit_area_and_limit_direction_with_pad I removed most of the code that I have added in this branch and will add it back in step by step in the next commits

Tests should be red now

cchwala · 2020-08-26T13:30:45Z

I am now looking into the suggestion from @simonjayhawkins to use interpolate_2d instead of introducing interpolate_1d_fill to add less clutter to the already cluttered code.

provided by pandas-dev#34749 Test are green now

cchwala · 2020-08-26T22:09:08Z

@jreback can you please have another look? As expected there is not much left from this PR because parts of it have already been merged elsewhere. Most of your code review comments do not apply anymore, only this one.

cchwala · 2020-08-27T20:31:37Z

@jreback Sorry to ping you again, but it would be great if I could finish this PR with (hopefully) final adjustments tomorrow (Friday). Otherwise things would have to wait again another two weeks because I will be offline.

jreback · 2020-08-28T02:08:37Z

will have to wait as i am away

cchwala · 2020-08-28T07:02:33Z

Okay. No problem.

jreback · 2020-09-06T18:15:23Z

pandas/core/internals/blocks.py

            limit=limit,
            fill_value=fill_value,
            dtype=self.dtype,
        )

+        if limit_area is None:


can you put this in a function in missing and just call here?

Okay. But if I do this I could also directly integrate it into missing.interpolate_2d, e.g. like in this branch. Advantage, no need to introduce yet another function that does some interpolation. Disadvantage, obfuscates missing.interpolate_2d. For this, adding some comments would help for sure.

If you prefer the separate function in missing, what function name would you choose?

e.g. like in this branch.

I would merge recursive solution here for review (so that ci has been run against it, i think needs changes to pandas/core/arrays/categorical.py see master...simonjayhawkins:fix_interpolate_limit_area_and_limit_direction_with_pad)

Okay. Merged and CI is running.

I have also pushed a branch with an example implementation using a separate function in missing. The name of the new function is not good, but the implementation would probably look something like this.

For both solutions, tests for interpolation pass, but I did not run the full test suite. Will see what changes are required when CI is done.

Test are green.

If you find the recursive solution appropriate, I would add a comment to the function that briefly explains the reasoning. Anything else that needs improvement?

… into interpolate_2d

simonjayhawkins

Thanks @cchwala can you add the type annotations as requested by @WillAyd https://github.com/pandas-dev/pandas/pull/31048/files#r367123208

simonjayhawkins · 2020-11-12T10:57:56Z

@cchwala can you resolve merge conflicts and can take another look

jreback

looks ok, some comments, can you merge master and we can get this in.

jreback · 2020-11-18T18:29:28Z

pandas/core/missing.py

+    # and `limit_area` logic along a certain axis.
+    if limit_area is not None:
+
+        def func(values):


can you make this a module level function and just call it here

jreback · 2020-11-24T13:50:38Z

moving off 1.2 need merge master and update to comments.

arw2019 · 2020-11-27T05:06:52Z

Closing in favor of #38106

cchwala added 3 commits January 15, 2020 17:02

fix test

fd5d8e8

This was referenced Jan 15, 2020

ENH: Added max_gap keyword for series.interpolate #25141

Closed

df.interpolate(method='pad') axis is not consistent with df.fillna(method='pad') #29146

Closed

cchwala added 4 commits January 15, 2020 22:06

pep8

26d88ed

fixed small error that actually had no effect since the input array `…

6597aca

…values` also was changed via appliying `func`

Raise when forbidden combination of method and limit_direction ar…

c536d3c

…e used Test for all forbidden combos of `pad` and `backfill` is included

Updated docstring with info about allowed combinations of method an…

ed9cf21

…d `limit_direction`

cchwala changed the title ~~FIX: fix interpolate with kward limit area and limit direction usung pad or bfill~~ FIX: fix interpolate with kwarg limit area and limit direction using pad or bfill Jan 15, 2020

clean up

2980325

* black formatting * typo

WillAyd requested changes Jan 15, 2020

View reviewed changes

WillAyd added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Jan 15, 2020

cchwala added 4 commits January 15, 2020 22:55

Added entry to whatsnew file

ecf428e

Removed axis kwarg from interpolate_1d_fill because it was unused

f8a3423

Type annotations added to new function interpolate_1d_fill

6733186

fixed incorrectly sorted imports

c5b77d2

cchwala requested a review from WillAyd January 15, 2020 22:49

WillAyd reviewed Jan 17, 2020

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

jreback requested changes Jan 18, 2020

View reviewed changes

Added type annotation, updated docstring and removed unnecessary argu…

0bb36de

…ments Test on my local machine are not affected by removing the unncessery arguments `valid` and `invalid`, which are now derived within the function.

cchwala added 3 commits February 18, 2020 20:41

Reverting docstring entry for default value of limit_direction

a467afd

Moved logic for calling missing.interpolate_1d_fill to `missing.int…

5466d8c

…erpolate_2d`

Moved whatsnew entry to v1.1.0.rst

3e968fc

simonjayhawkins mentioned this pull request Aug 25, 2020

Fix interpolate limit area and limit direction with pad #35893

Closed

cchwala added 2 commits August 25, 2020 21:52

Merge remote-tracking branch 'upstream/master' into fix_interpolate_l…

8ceff58

…imit_area_and_limit_direction_with_pad I removed most of the code that I have added in this branch and will add it back in step by step in the next commits

Added tests back in

7c5ad7d

Tests should be red now

cchwala added 3 commits August 26, 2020 22:25

Added new solution to account for limit_area with pad

92148ff

provided by pandas-dev#34749 Test are green now

black formatting

d62e02e

added whatsnew entry

c2473f2

cchwala requested a review from jreback August 26, 2020 22:09

jreback requested changes Sep 6, 2020

View reviewed changes

cchwala added 2 commits September 15, 2020 14:17

Test with moving logic for interpolate_2d with limite_area directly…

610e347

… into interpolate_2d

fix wrong arg order by using kwargs as suggested in pandas-dev@75f3804

570e3c2

simonjayhawkins requested changes Sep 15, 2020

View reviewed changes

simonjayhawkins added this to the 1.2 milestone Sep 15, 2020

cchwala added 3 commits September 15, 2020 16:26

Added comment to explain recursion and added typing for interpolate_2d

721304a

improved test code coverage

73ab1bf

fixed wrong typing

a33f629

cchwala requested a review from jreback September 17, 2020 10:36

jreback requested changes Nov 18, 2020

View reviewed changes

jreback removed this from the 1.2 milestone Nov 24, 2020

arw2019 mentioned this pull request Nov 27, 2020

BUG in Series.interpolate: limit_area/limit_direction kwargs with method="pad"/"bfill" have no effect #38106

Merged

5 tasks

arw2019 closed this Nov 27, 2020

		@@ -478,6 +500,66 @@ def _akima_interpolate(xi, yi, x, der=0, axis=0):
		return [P(x, nu) for nu in der]


		def interpolate_1d_fill(

FIX: fix interpolate with kwarg limit area and limit direction using pad or bfill #31048

FIX: fix interpolate with kwarg limit area and limit direction using pad or bfill #31048

Conversation

cchwala commented Jan 15, 2020 • edited Loading

pep8speaks commented Jan 15, 2020 • edited Loading

Comment last updated at 2020-09-15 15:40:12 UTC

WillAyd commented Jan 15, 2020

cchwala commented Jan 15, 2020

WillAyd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchwala commented Jan 15, 2020

cchwala commented Jan 16, 2020 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchwala Feb 18, 2020 • edited Loading

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

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

WillAyd commented Feb 12, 2020

simonjayhawkins commented Aug 25, 2020

cchwala commented Aug 25, 2020

cchwala commented Aug 26, 2020 • edited Loading

cchwala commented Aug 26, 2020 • edited Loading

cchwala commented Aug 27, 2020

jreback commented Aug 28, 2020

cchwala commented Aug 28, 2020

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

Choose a reason for hiding this comment

simonjayhawkins left a comment

Choose a reason for hiding this comment

simonjayhawkins commented Nov 12, 2020

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 24, 2020

arw2019 commented Nov 27, 2020

cchwala commented Jan 15, 2020 •

edited

Loading

pep8speaks commented Jan 15, 2020 •

edited

Loading

cchwala commented Jan 16, 2020 •

edited

Loading

cchwala Feb 18, 2020 •

edited

Loading

cchwala commented Aug 26, 2020 •

edited

Loading

cchwala commented Aug 26, 2020 •

edited

Loading