BUG/ENH: Add how kwarg to rolling_* functions [fix #6297]

[jsexauer]

Fixes #6297

Figured while I was in the area, would submit a PR to do this too. Let me know if you want to see more unit tests than the one.

[jreback]

median should also have a how=median I think

pls add a similar test for min/ median as well

the docs in computation.rst might need a bit of tweaking as well

[jsexauer]

Updated to include median, additional tests, and added documentation in computation.rst

[jreback]

can u add a note near the how that u updated in computation.rst to explain when the how is used (eg only in intra day resample a when the moment freq is > day)

[jorisvandenbossche]

Expressing a concern: I don't know if I find this a very good idea. Some things that worry me:

• I think it is actually a lot more explicit to just ask the user to do the resample themselves, instead of adding yet an extra argument and trying to do two things in one. So for example instead of doing:

  pd.rolling_max(series, window=1, freq='D', how='max')
  

  just do

  pd.rolling_max(series.resample(freq='D', how='max'), window=1)
  

  This is a little longer, but personally I find this more explicit and clear what is going on for someone reading this code.

• as was noted in the issue, it is not really a bug, but just a strange implementation (but a documented one). And changing the default value for how to max for max or median for median, etc can also possibly break code, and given my previous point, I don't know if this is worth it.

• Maybe the main reason why I am hesitant about this is that I have always found this freq keyword argument very confusing (although it is clear what it does, the explanation in the docstring was in the past just totally not clear, but this is better now). When I first encountered this, I expected that I could use freq to specify the window as a frequency when having timeseries data (eg a frequency of '15min' instead of window=15 if your data are every minute). A functionality that I would find much more useful.

But it's maybe just a stupid feeling, and you can certainly convince me that this is a good idea. Even now writing it down my hesitation is already not that strong anymore :-)

[jorisvandenbossche]

Hmm, thinking more thoroughly about it, with some use case in my head. If you have an irregular timeseries, it is indeed resampling it to a regular timeseries what you want, otherwise the rolling function does not make much sense. My idea of specifying the window as a frequency when not having a regular timeseries is also not really well defined what this should do, without first resampling it.
So forget what I said. This functionality is indeed useful within the rolling_ functions.

Concern of breaking backwards compatibility still holds, but maybe not too many people will rely on this? (for min/max/median) It is also not really possible to do this with a deprecation warning I think.

[jorisvandenbossche]

Some spaces are missing at the beginning of the line to align this with the previous lines.

[jorisvandenbossche]

Added some comments on the docs.

For the release notes. I think there should also be a mention in the API changes section? (for the change of the default for min/max/median)

[jreback]

@jorisvandenbossche I think this actually a bug (but can put in the API section)
if u want a rolling_max then it should really be the max and not the max of a meaned daily number which does not make much sense

only affects max,min,median

[jorisvandenbossche]

@jreback Yeah, I agree after some better thinking that is normally (in most cases) not what you want, it's just that is was documented. So in that sense certainly an api change.

But you can forget my concerns :-)

[jreback]

@jsexauer can you rebase on master; looks fine otherwise

@jorisvandenbossche ?

ping when ready

[jorisvandenbossche]

Yes, I am OK with this, just my two formatting comments of above.

[jsexauer]

@jreback Ready for merging.

[jreback]

ok...ping when green

[jsexauer]

It is green...?

[jreback]

thanks!