-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored Resample API breaking change #11841
Conversation
Although I really like the fact of a more consistent API, I think this is so backwards incompatible that it really is problematic to just put it in a release like this. I also kind of like the simplicity of a basic resample to explain in tutorials (although once it is not a basic resample anymore, this interface is nicer). A two-step method is more complicated than a 1-step.. Regarding to the back compat issue, there are maybe other ways to solve this? Eg, I think of using another name, some keyword indicating the behaviour (but this will get ugly), .... |
pls explain. The point of breaking compat is to change future behavior. breaking changes always have short term pain, but shying away from actual long time needed inconsistencies is MUCH MUCH worse. |
yes we could add a |
One thing we could do to help preserve backwards compat is to keep around |
I already do the deprecation warning (and just return the result) if The only case this is actually breaking is if NO
|
This might be too magical and/or tricky to pull off, but we might add fallback methods to |
hacking
|
86b47f2
to
008c7b4
Compare
@jreback nice. One thing we'll have to catch explicitly is |
so defined all of the arithmetic ops / comparison ops and instance checking
|
so its actually quite a tricky problem to catch this. imagine you had this code originally
The warning will show up, but the setting is actually on a copy. I prob should just raise. |
Indeed, this needs to raise. I would say this raising is more important than adding the Series/DataFrame facade. If we don't have a facade, things will fail loudly, which is unfortunate, but if we have a broken facade, things will fail silently! |
@jorisvandenbossche how are we removing deprecation warnings on older whatsnew files? |
@jreback See #6856 (comment) for general discussion on this. For now, adding |
To answer this: of course we have to keep a right balance between avoiding breaking changes and improving pandas. But that is not always very clear what the right balance is, and I could also imagine other parts of pandas that could use some breaking changes, but that we are reluctant about to do because of its impact (not to mention the indexing api ... :-)) But, as you now updated to include a deprecation warning or fail with an informative error, it sounds already a lot better to me! (at least there will be no silent failure, that's probably the most important) Maybe I will ping the mailing list about this, as it is a rather large change? |
@jorisvandenbossche you e-mail to the list is good. thanks. I agree about most changes, but some things just need to be more consistent, this and the window functions are in the groupby pattern (which is quite in-grained in pandas). So to me this is a no-brainer, EVEN if we had to break back-compat (in this case was able to provide a nice easy upgrade path so that makes this pretty easy to swallow IMHO). if you want to review, this is also ready to go. |
cae173b
to
b83dae1
Compare
Some more usage feedback:
|
Ah, the |
yes the |
@jorisvandenbossche ok, latest push should fix everything you mentioned here (except for a couple of issues I have marked in #12140) |
2cccc70
to
329567c
Compare
@jorisvandenbossche if you'd have a final look. |
any more comments @jorisvandenbossche ? |
@jreback Thanks for all the edits based on my comments. I quickly looked at some of them, and all looks good! Only responded to two of them (both about the whatsnew). I don't have the time at the moment to take a final look, but I trust my previous rounds of comments and your edits that this is good to go! My only issue is still the behavior with dicts in |
original API detection & warning support for isinstance / numeric ops support for comparison ops DOC: documentation updates w.r.t. aggregation
PEP on pandas/core/groupby.py
@jorisvandenbossche ok, I took out the aggregation clarification docs, they are the same as existing (my mistake), expect for a slightly better error message. The behavior has not changed AFICT from 0.17.1. |
yay! let's get |
on top of #11603closes #11732
closes #12072
closes #9052
closes #12140
ToDo:
New API
Upsampling