ENH: Poisson exponential window #26243

rbenes · 2019-04-29T20:23:07Z

closes Poisson (exponential) window for pandas.Series.rolling #21303
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

rbenes · 2019-04-29T20:26:06Z

pandas/core/window.py

-                           'slepian': ['width']}
+                           'slepian': ['width'],
+                           'exponential': ['center', 'tau']}
+                immutable_args_map = {'exponential': {'center': None}}


this map is here because window param in get_window function (https://docs.scipy.org/doc/scipy-0.19.1/reference/generated/scipy.signal.get_window.html) for exponential window (https://docs.scipy.org/doc/scipy-0.19.1/reference/generated/scipy.signal.exponential.html#scipy.signal.exponential) needs parameters center and tau. But we do not want to set center here.

Not sure I completely follow - do you have a test case that is supposed to hit this?

I will try to describe it more deeply. Maybe we will find a better solution.

Window is created by calling sig.get_window (

pandas/pandas/core/window.py

Line 655 in 3876dd8

return sig.get_window(win_type, window, False).astype(float)

). For this function, you have to prepare tuple win_type that consists of few items (win_name, win_param_1, win_param_2, ...) - it was already implemented and works perfectly for previously supproted windows.
For example for gaussinan window, you have to call scipy function sig.get_window like this w = sig.get_window(("gaussian", 1), N, True). The tuple ("gaussian", 1) is inside scipy.singal parsed and appropriate function is then called e.g. here for gaussian is called scipy.signal.gaussian(M, std, sym=True) -> scipy.signal.gaussian(N, 1)

The exponential window is defined like this scipy.signal.exponential(M, center=None, tau=1.0, sym=True), and the problem is in center parameter. I need to prepare win_type (the tuple) that contains None as the first window parameter, but is not user parameter passed in kwargs (and should not be). So for exponential window, I need the tuple like this ('exponential', None, tau). This is the main complexity here that I need to add aditional param that is not passed by user in kwargs.

About the test. The immutable_args_map functionality is tested when test_cmov_window_special is called for exponential window.

Hmm...it feels somewhat special-case-y, but this seems to be borne out of scipy API. I don't feel great about this, but I do understand the necessity.

I checked all possible windows in scipy once again and I did not find another window, where we would use this approach. For other windows (if they will be in future implemented), the parameters exposed in pandas will be the same as the ones passed to underlying scipy function sig.get_window(). So maybe this immutable_args_map is overkill here. I will try to come another (more lightweight) solution.

codecov · 2019-04-29T21:01:06Z

Codecov Report

Merging #26243 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26243      +/-   ##
==========================================
- Coverage   91.97%   91.96%   -0.01%     
==========================================
  Files         175      175              
  Lines       52368    52373       +5     
==========================================
+ Hits        48164    48166       +2     
- Misses       4204     4207       +3

Flag	Coverage Δ
#multiple	`90.52% <100%> (ø)`	⬆️
#single	`40.68% <0%> (-0.16%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.42% <100%> (+0.02%)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`96.9% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.71% <0%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9feb3ad...3876dd8. Read the comment docs.

codecov · 2019-04-29T21:01:11Z

Codecov Report

Merging #26243 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26243      +/-   ##
==========================================
- Coverage   91.98%   91.98%   -0.01%     
==========================================
  Files         175      175              
  Lines       52374    52380       +6     
==========================================
+ Hits        48178    48182       +4     
- Misses       4196     4198       +2

Flag	Coverage Δ
#multiple	`90.54% <100%> (ø)`	⬆️
#single	`40.77% <0%> (-0.1%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.41% <100%> (+0.01%)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`96.9% <0%> (-0.12%)`	⬇️
pandas/core/algorithms.py	`94.73% <0%> (-0.07%)`	⬇️
pandas/core/config_init.py	`96.94% <0%> (-0.03%)`	⬇️
pandas/core/base.py	`97.98% <0%> (ø)`	⬆️
pandas/core/sorting.py	`98.35% <0%> (+0.06%)`	⬆️
pandas/util/testing.py	`90.7% <0%> (+0.09%)`	⬆️
pandas/core/tools/datetimes.py	`85.05% <0%> (+0.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94c8c94...af96493. Read the comment docs.

WillAyd · 2019-04-30T01:41:27Z

pandas/core/window.py

-                           'slepian': ['width']}
+                           'slepian': ['width'],
+                           'exponential': ['center', 'tau']}
+                immutable_args_map = {'exponential': {'center': None}}


Not sure I completely follow - do you have a test case that is supposed to hit this?

jreback · 2019-04-30T10:27:35Z

pandas/core/window.py

+                           'slepian': ['width'],
+                           'exponential': ['center', 'tau']}
+                immutable_args_map = {'exponential': {'center': None}}
+
                if win_type in arg_map:


why are you changing the structure of passing args here? was there some reason the existing would not just work?

For exponential window kwargs contains {tau: 10} but I need to get back tuple (None, 10) (first None is the center parameter required for exponential window). So I pass the immutable_args to _pop_args function that is the second source of params (another than kwargs).

Another possible option can be add {"center": None} to kwargs for exponential window and let _pop_args without change.
More info about exponential window is in comments above.

yes pls do this instead

The code is now without immutable_args. As I wrote in another comment, I simplyfied my code even more because other window (that would be possibly implemented in future) in scipy will not need such handling like the exponential one. So please check the last code version.
Thanks.

WillAyd

Last commit looks a lot better so thanks! Just as a general question, is there no need whatsoever for a user to pass center through kwargs? Seems like this one-off handling might prevent that from going through if there's a legitimate use for it?

rbenes · 2019-05-05T18:59:17Z

Last commit looks a lot better so thanks! Just as a general question, is there no need whatsoever for a user to pass center through kwargs? Seems like this one-off handling might prevent that from going through if there's a legitimate use for it?

Yes, user cannot pass center argument at all. It is forbidden because here:

pandas/pandas/core/window.py

Line 644 in 48ea04f

return sig.get_window(win_type, window, False).astype(float)

we pass last argument as False - that means that this function creates symetric window. For exponential window (https://docs.scipy.org/doc/scipy-0.19.1/reference/generated/scipy.signal.exponential.html), if you set sym=True, you cannot set center param (its in documentation)

In [1]: import scipy.signal as sig                                                                                                              

In [2]: w = sig.get_window(("exponential", 1, 10), 5, False)                                                                                    
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-0fd28a7b1563> in <module>
----> 1 w = sig.get_window(("exponential", 1, 10), 5, False)

~/envs/pandas/lib/python3.6/site-packages/scipy/signal/windows/windows.py in get_window(window, Nx, fftbins)
   2105         params = (Nx, beta, sym)
   2106 
-> 2107     return winfunc(*params)

~/envs/pandas/lib/python3.6/site-packages/scipy/signal/windows/windows.py in exponential(M, center, tau, sym)
   1693     """
   1694     if sym and center is not None:
-> 1695         raise ValueError("If sym==True, center must be None.")
   1696     if _len_guards(M):
   1697         return np.ones(M)

ValueError: If sym==True, center must be None.

In [3]: w = sig.get_window(("exponential", None, 10), 5, False)                                                                                 

In [4]: w                                                                                                                                       
Out[4]: array([0.81873075, 0.90483742, 1.        , 0.90483742, 0.81873075])

jreback · 2019-05-05T21:22:56Z

pandas/core/window.py

+                           'slepian': ['width'],
+                           'exponential': ['center', 'tau']}
+                immutable_args_map = {'exponential': {'center': None}}
+
                if win_type in arg_map:


yes pls do this instead

jreback · 2019-05-05T21:23:16Z

also a conflict in 0.25.0 whatsnew

rbenes · 2019-05-06T19:01:26Z

also a conflict in 0.25.0 whatsnew

Rebased, conflict solved

jreback

lgtm. can you also update the docs: http://pandas-docs.github.io/pandas-docs-travis/user_guide/computation.html#window-functions (at the end of the section)

rbenes · 2019-05-07T05:31:49Z

lgtm. can you also update the docs: http://pandas-docs.github.io/pandas-docs-travis/user_guide/computation.html#window-functions (at the end of the section)

Updated.

jreback · 2019-05-07T11:31:45Z

thanks @rbenes

rbenes commented Apr 29, 2019

View reviewed changes

WillAyd requested changes Apr 30, 2019

View reviewed changes

WillAyd added the Window rolling, ewma, expanding label Apr 30, 2019

jreback changed the title ~~ENH: Poison exponential window~~ ENH: Poisson exponential window Apr 30, 2019

jreback requested changes Apr 30, 2019

View reviewed changes

gfyoung added the Enhancement label May 2, 2019

WillAyd reviewed May 5, 2019

View reviewed changes

jreback requested changes May 5, 2019

View reviewed changes

rbenes added 3 commits May 6, 2019 20:58

Exponential window can be used

99f498b

Whats new added

1b3d957

Simplify

d867ad4

rbenes force-pushed the poison_exponential_window branch from 33e59e7 to d867ad4 Compare May 6, 2019 19:00

jreback requested changes May 7, 2019

View reviewed changes

computation.rst updated

af96493

jreback added this to the 0.25.0 milestone May 7, 2019

jreback approved these changes May 7, 2019

View reviewed changes

jreback merged commit 2bbc0c2 into pandas-dev:master May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Poisson exponential window #26243

ENH: Poisson exponential window #26243

rbenes commented Apr 29, 2019 •

edited

Loading

rbenes Apr 29, 2019

WillAyd Apr 30, 2019

rbenes Apr 30, 2019

gfyoung May 2, 2019

rbenes May 3, 2019

codecov bot commented Apr 29, 2019

codecov bot commented Apr 29, 2019 •

edited

Loading

WillAyd Apr 30, 2019

jreback Apr 30, 2019

rbenes Apr 30, 2019

jreback May 5, 2019

rbenes May 6, 2019

WillAyd left a comment

rbenes commented May 5, 2019

jreback May 5, 2019

jreback commented May 5, 2019 •

edited

Loading

rbenes commented May 6, 2019

jreback left a comment

rbenes commented May 7, 2019

jreback commented May 7, 2019

ENH: Poisson exponential window #26243

ENH: Poisson exponential window #26243

Conversation

rbenes commented Apr 29, 2019 • 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

codecov bot commented Apr 29, 2019

Codecov Report

codecov bot commented Apr 29, 2019 • edited Loading

Codecov Report

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 left a comment

Choose a reason for hiding this comment

rbenes commented May 5, 2019

Choose a reason for hiding this comment

jreback commented May 5, 2019 • edited Loading

rbenes commented May 6, 2019

jreback left a comment

Choose a reason for hiding this comment

rbenes commented May 7, 2019

jreback commented May 7, 2019

rbenes commented Apr 29, 2019 •

edited

Loading

codecov bot commented Apr 29, 2019 •

edited

Loading

jreback commented May 5, 2019 •

edited

Loading