Navigation Menu

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

INT/CLN: remove to_datetime from class methods #8254

Closed
5 tasks done
jreback opened this issue Sep 12, 2014 · 17 comments
Closed
5 tasks done

INT/CLN: remove to_datetime from class methods #8254

jreback opened this issue Sep 12, 2014 · 17 comments
Labels
API Design Internals Related to non-user accessible pandas implementation IO CSV read_csv, to_csv
Milestone

Comments

@jreback
Copy link
Contributor

jreback commented Sep 12, 2014

see #8184
related #8185

Index.to_datetime currently exists but really no need for this as pd.to_datetime subsumes this functionaility (and is more general). and just pollutes the interface.

so remove:

(and fix the parser to call directly try the conversion / use a helper function).

@jreback jreback added API Design IO CSV read_csv, to_csv Internals Related to non-user accessible pandas implementation labels Sep 12, 2014
@jreback jreback added this to the 0.16 milestone Sep 12, 2014
@jreback jreback changed the title INT/CLN: remove to_datetime/to_timedelta from class methods INT/CLN: remove to_datetime from class methods Sep 12, 2014
@jreback jreback modified the milestones: 0.16, 0.15.1 Oct 7, 2014
@jreback jreback modified the milestones: 0.16.0, Next Major Release Mar 6, 2015
@gfyoung
Copy link
Member

gfyoung commented Aug 27, 2016

I actually would like to keep Timestamp.to_datetime. That's useful function I've found for converting from native pandas datetime objects to generic Python datetime ones. In addition, pd.to_datetime only returns Timestamp and not datetime.

@jorisvandenbossche
Copy link
Member

See also #5612 (wihch can be closed as well when this is deprecated)

I actually would like to keep Timestamp.to_datetime

There is Timestamp.to_pydatetime that does the same and is more consistent with the name for DatetimeIndex etc

@gfyoung
Copy link
Member

gfyoung commented Aug 27, 2016

@jorisvandenbossche : Ah, that's a good point. I wasn't actually familiar with the to_pydatetime method, though given what you said about consistency, we should look into deprecating that.

gfyoung added a commit to forking-repos/pandas that referenced this issue Aug 27, 2016
Partially addresses pandas-devgh-8254.
Closes pandas-devgh-8612 because pd.to_datetime has a format arg.

[ci skip]
jorisvandenbossche pushed a commit that referenced this issue Aug 27, 2016
Partially addresses gh-8254.
Closes gh-8612 because pd.to_datetime has a format arg.
@gfyoung
Copy link
Member

gfyoung commented Aug 27, 2016

@jorisvandenbossche : On the topic of deprecating to_datetime, any reason why we need to keep NaT.to_datetime when pd.to_datetime(NaT) will do just fine?

@jreback
Copy link
Contributor Author

jreback commented Aug 27, 2016

I would remove Timestamp.to_datetime as well (well deprecate it).

@jreback jreback modified the milestones: 0.19.0, Next Major Release Aug 27, 2016
@gfyoung
Copy link
Member

gfyoung commented Aug 27, 2016

@jreback : Yes, that I can now agree with given what @jorisvandenbossche said earlier. What about NaT.to_datetime?

@jreback
Copy link
Contributor Author

jreback commented Aug 27, 2016

yes that as well. no vestages, check Period as well pls.

@gfyoung
Copy link
Member

gfyoung commented Aug 27, 2016

@jreback : Period has no such method 😄

@gfyoung
Copy link
Member

gfyoung commented Aug 27, 2016

Also, checkoff the Index and DatetimeIndex boxes but add one for NaT.

gfyoung added a commit to forking-repos/pandas that referenced this issue Aug 28, 2016
@gfyoung
Copy link
Member

gfyoung commented Aug 28, 2016

I just discovered that PeriodIndex has a to_datetime method as well. However, this case is somewhat unique because you can't call pd.to_datetime on it (an error is raised because it is the wrong type). Nevertheless, PeriodIndex.to_datetime does convert to DatetimeIndex and hence behaves similarly to pd.to_datetime.

The implementation for PeriodIndex.to_datetime seems quite tailored to PeriodIndex (it calls a special helper to_timestamp) and to port it to the general to_datetime does not seem optimal. Thoughts on how this should be refactored so that PeriodIndex.to_datetime can be deprecated?

@jreback
Copy link
Contributor Author

jreback commented Aug 28, 2016

sure this can be removed
that is the point of to_timestamp which is the canonical way

@gfyoung
Copy link
Member

gfyoung commented Aug 28, 2016

Let's add it to the list then since it will require slightly different treatment.

@jorisvandenbossche
Copy link
Member

Is it needed to remove the PeriodIndex.to_datetime? As that seems a valid method to me, as the counterpart of DatetimeIndex.to_period ?

@jorisvandenbossche
Copy link
Member

Ah, I see that this functionality is covered by PeriodIndex.to_timestamp (sorry, should have looked better to your comment).
But, personally, I like the to_datetime in this case, as it converts to a 'Datetime'Index (the current docstring of PeriodIndex.to_datetime is in any case totally incorrect)

@gfyoung
Copy link
Member

gfyoung commented Aug 28, 2016

@jorisvandenbossche : Agreed, the doc string is not right because it inherits it from Index. IMO pd.to_datetime should be the only to_datetime method exposed to unify the interface.

One option is to add a check for PeriodIndex in to pd_datetime so that it calls PeriodIndex.to_timestamp (currently, an error is raised). Thoughts?

@jorisvandenbossche
Copy link
Member

I see that there is also a Series.to_timestamp (and no Series.to_datetime) to convert period index to datetime index. So probably for consistency, to_timestamp is indeed to be preferred and OK with deprecating PeriodIndex.to_datetime

(accepting Periods in to_datetime is a separate issue I think)

@gfyoung
Copy link
Member

gfyoung commented Aug 28, 2016

Ah, okay, fair enough. Will leave that part alone then.

jorisvandenbossche pushed a commit that referenced this issue Aug 29, 2016
* DEPR: Deprecate Timestamp.to_datetime

* API: Issue real warning in to_pydatetime

* DEPR: Deprecate NaT.to_datetime

Closes gh-8254.
gfyoung added a commit to forking-repos/pandas that referenced this issue Dec 5, 2017
Removes the following .to_datetime methods

* Index.to_datetime
* Timestamp.to_datetime
* PeriodIndex.to_datetime
* DatetimeIndex.to_datetime

All were deprecated in 0.19.0

xref pandas-devgh-8254, pandas-devgh-14096, pandas-devgh-14113
gfyoung added a commit to forking-repos/pandas that referenced this issue Dec 6, 2017
Removes the following .to_datetime methods

* Index.to_datetime
* NaTType.to_datetime
* Timestamp.to_datetime
* PeriodIndex.to_datetime
* DatetimeIndex.to_datetime

All were deprecated in 0.19.0

xref pandas-devgh-8254, pandas-devgh-14096, pandas-devgh-14113
gfyoung added a commit that referenced this issue Dec 6, 2017
Removes the following .to_datetime methods

* Index.to_datetime
* NaTType.to_datetime
* Timestamp.to_datetime
* PeriodIndex.to_datetime
* DatetimeIndex.to_datetime

All were deprecated in 0.19.0

xref gh-8254, gh-14096, gh-14113
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design Internals Related to non-user accessible pandas implementation IO CSV read_csv, to_csv
Projects
None yet
Development

No branches or pull requests

3 participants