Skip to content
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

CLN: Removed coerce param in pd.to_timedelta and pd.to_datetime #13602

Closed

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Jul 9, 2016

Deprecated back in v0.17.0 here. Seems appropriate to carry though now.

@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch from 9f0781d to 2d5eaeb Compare July 9, 2016 21:52
Removal of prior version deprecations/changes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

- ``pd.to_timedelta`` has dropped the ``coerce`` parameter (:issue:`13602`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add what it's in favor of

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Old build to cancel here.

@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch from 2d5eaeb to 76e0bcd Compare July 9, 2016 21:59
@codecov-io
Copy link

codecov-io commented Jul 10, 2016

Current coverage is 85.30% (diff: 100%)

Merging #13602 into master will increase coverage by 0.04%

@@             master     #13602   diff @@
==========================================
  Files           140        139     -1   
  Lines         50634      50139   -495   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          43173      42771   -402   
+ Misses         7461       7368    -93   
  Partials          0          0          

Powered by Codecov. Last update d98e982...c8c5f49

@gfyoung
Copy link
Member Author

gfyoung commented Jul 10, 2016

@jreback : Travis is passing, so ready to merge if there are no other concerns.

@jreback
Copy link
Contributor

jreback commented Jul 10, 2016

actually, I don't think this can be done w/o doing for to_datetime as well. Either both or none.

@jreback jreback added Timedelta Timedelta data type Deprecate Functionality to remove in pandas labels Jul 10, 2016
@jreback
Copy link
Contributor

jreback commented Jul 10, 2016

and we generally are waiting much longer, see the list @jorisvandenbossche put up at the very bottom:
#6581

any of those are fair game. Yes we could/should do some of the 0.17.0 ones, but let's get some of the others first.

@gfyoung
Copy link
Member Author

gfyoung commented Jul 10, 2016

@jreback : so if I also remove the to_datetime param, will this be merged in for 0.19.0 or not? A duration of two major releases seems like a good amount of time for people to adjust their code I thought.

@jreback
Copy link
Contributor

jreback commented Jul 10, 2016

@gfyoung yes it is normally ok, but we SO many deprecations (esp from 0.17.0.), that I think let's clean everything up thats < 0.17.0 first. So I would say defer this to 0.20.0..

@gfyoung
Copy link
Member Author

gfyoung commented Jul 10, 2016

@jreback : Hmm...I'm not sure I totally follow that logic, especially since I do see a deprecation removal from 0.17.0 being listed in that issue that you linked to (the legacy offsets one).

@jreback
Copy link
Contributor

jreback commented Jul 10, 2016

yeah. I don't have a problem with anything from 0.17.0. BUT this is a 'biggie' (well to_datetime is really). So no objection if you include both (for consistency). But if you have a limited pool of PR energy, would prefer other ones to be addressed .....

@gfyoung
Copy link
Member Author

gfyoung commented Jul 10, 2016

@jreback : this one isn't that difficult to remove AFAICT - I'll also take a look at some of the other deprecation removals and put up PR's for those if I can.

@jreback
Copy link
Contributor

jreback commented Jul 10, 2016

right, add on to_datetime removal then would be ok, but @jorisvandenbossche might feel different.

@gfyoung
Copy link
Member Author

gfyoung commented Jul 10, 2016

@jreback : fair enough - in the meantime, I think we do have one deprecation removal that I think all of us can agree upon (#13419) 😄

@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch 3 times, most recently from d0e4b05 to a0455c8 Compare July 12, 2016 02:26
@gfyoung
Copy link
Member Author

gfyoung commented Jul 12, 2016

@jreback : removed the coerce parameter in to_datetime, and Travis is still passing. @jorisvandenbossche thoughts?

@jorisvandenbossche
Copy link
Member

Personally I would table this for 0.20 ..
The other deprecated things we are removing are all even from 0.15 and (the maybe's) from 0.16 (except for the offset/freq PR), and the 0.17 release is less than a year old.

@jorisvandenbossche
Copy link
Member

Maybe we need some more 'formal' rules on how long deprecations are kept at the minimum? Eg minimal two feature releases and minimal a year.

@gfyoung
Copy link
Member Author

gfyoung commented Jul 13, 2016

@jorisvandenbossche: When has two feature releases ever taken more than a year? Two feature releases sure, but a year seems a little long too me, and not just because I would like to merge this ;)

@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch from a0455c8 to 8f3e8a3 Compare July 13, 2016 02:07
@jreback
Copy link
Contributor

jreback commented Jul 13, 2016

I think let's defer this one to 0.20. I don't see any downside to delaying this.

@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch from 8f3e8a3 to ed5f093 Compare July 13, 2016 06:03
@jorisvandenbossche jorisvandenbossche added this to the 0.20.0 milestone Jul 13, 2016
@jorisvandenbossche
Copy link
Member

but a year seems a little long too me, and not just because I would like to merge this ;)

We may not like it, but it is a fact that not all users upgrade each version but often skip a few at once ...
I understand you rather see it merged, but rebasing it in a few months will also not be that much work!

@gfyoung
Copy link
Member Author

gfyoung commented Jul 13, 2016

@jorisvandenbossche : this deprecation removal process is somewhat new to me, and I was not aware of user tendencies to skip version upgrades (I generally follow them quite closely for my projects). However, I perfectly understand! I was being somewhat facetious when I mentioned that I wanted to merge this now from the ;) I added at the end.

@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch 4 times, most recently from a9c191f to a799398 Compare September 24, 2016 19:10
@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch 6 times, most recently from 0d6aae4 to e3c54f5 Compare October 2, 2016 20:19
@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch 3 times, most recently from 0a968ed to 18c4f9d Compare October 9, 2016 19:25
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK to me!

@gfyoung gfyoung force-pushed the remove-coerce-to-timedelta branch 2 times, most recently from 1363567 to eb62e50 Compare October 13, 2016 14:16
@jorisvandenbossche
Copy link
Member

@gfyoung can you force push again? (possibly due to the repo transfer the PRs open before the transfer cannot be merged from the github interface)

@jreback
Copy link
Contributor

jreback commented Oct 13, 2016

@jorisvandenbossche if you merge this, we for sure need to branch 0.19.1 (which is ok)

@jreback
Copy link
Contributor

jreback commented Oct 13, 2016

also should remove coerce from pd.to_datetime for consistency.

@gfyoung
Copy link
Member Author

gfyoung commented Oct 13, 2016

@jreback : Ah, good point. I didn't realize it had been deprecated back in 0.17.0. Will do then.

@gfyoung
Copy link
Member Author

gfyoung commented Oct 14, 2016

@jreback : Actually, never mind! I did remove it in this PR! Need to change the title.

@gfyoung gfyoung changed the title CLN: Removed coerce param in pd.to_timedelta CLN: Removed coerce param in pd.to_timedelta and pd.to_datetime Oct 14, 2016
@jreback
Copy link
Contributor

jreback commented Oct 14, 2016

ok lgtm. our first non-0.19.1 PR!

@jorisvandenbossche
Copy link
Member

Merged! Thanks @gfyoung

@jreback
Copy link
Contributor

jreback commented Oct 15, 2016

@jorisvandenbossche so need to branch 0.19.1 (as this should not be included)!

tworec pushed a commit to RTBHOUSE/pandas that referenced this pull request Oct 21, 2016
Deprecated back in `v0.17.0` <a href="https://github.com/pydata/pandas
/commit/987b7e7e586b8df1d127406c69e0a9094a1a5322">here</a>.  Seems
appropriate to carry though now.

Author: gfyoung <gfyoung17@gmail.com>

Closes pandas-dev#13602 from gfyoung/remove-coerce-to-timedelta and squashes the following commits:

091af83 [gfyoung] CLN: Removed coerce param in pd.to_timedelta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants