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

API: remove datetime-like index ops from Series #7206

Merged
merged 2 commits into from
May 22, 2014
Merged

Conversation

jreback
Copy link
Contributor

@jreback jreback commented May 22, 2014

as per discussed in #7146. may provide a to_index() and/or an accessor in future versions

@jreback jreback added this to the 0.14.0 milestone May 22, 2014
@jreback
Copy link
Contributor Author

jreback commented May 22, 2014

@cpcloud @jorisvandenbossche ?

jreback added a commit that referenced this pull request May 22, 2014
API: remove datetime-like index ops from Series
@jreback jreback merged commit 4b17314 into pandas-dev:master May 22, 2014
- allow ``Series`` and ``Index`` to share common ops. remove the ``Series.weekday`` property from Series;
Using a ``DatetimeIndex/PeriodIndex`` method on a Series will now raise a ``TypeError``.
support ``min(),max(),factorize(),unique(),nunique(),value_counts()`` on ``Index`` types.
(:issue:`4551`, :issue:`4056`, :issue:`5519`, :issue:`6380`, :issue:`7206`).
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit cryptic I think. On the one hand you say that Series and Index share common operators, but then you say weekday is removed and datetimeindex methods on a series will raise (so this is not sharing).

Are the changes that are relevant to users not:

  • Series.weekday is removed (and actually should first be deprecated?)
  • min/max/unique/nunique/value_counts methods are added to Index types (that they are shared with Series is an implementation detail, no?)

@jorisvandenbossche
Copy link
Member

Just added a note about the new whatsnew entry, but for the rest am OK on disabling this for now.

@jreback
Copy link
Contributor Author

jreback commented May 22, 2014

@jorisvandenbossche fair comments

the issue is that it can become ambiguous, so don't want to add something w/o dealing with that.

Series.weekday wasn't documented at all so I don't think a big deal to remove.

@jorisvandenbossche
Copy link
Member

@jreback OK for the weekday

What do you mean with ambiguous? What can become ambiguous? min/max/unique/nunique/value_counts will not always act on the series values?

@jreback
Copy link
Contributor Author

jreback commented May 22, 2014

not the usage of Series.weekday could be ambiguous (and applies to all of the properties), IF the series contains a datetimeindex as well (or periodindex)

In [1]: s      = Series(pd.date_range('20130101',periods=3),index=pd.date_range('20130104',periods=3))

In [2]: s
Out[2]: 
2013-01-04   2013-01-01
2013-01-05   2013-01-02
2013-01-06   2013-01-03
Freq: D, dtype: datetime64[ns]

In [3]: s.index.weekday
Out[3]: array([4, 5, 6])

In [4]: pd.DatetimeIndex(s).weekday
Out[4]: array([1, 2, 3])

@jorisvandenbossche
Copy link
Member

ah yes OK, weekday is indeed ambiguous like the other datetime accessors. And so they are disabled again in this PR. That's all OK (but I thought you were speaking about the others)

@jreback
Copy link
Contributor Author

jreback commented May 22, 2014

no, the others were added (not in this PR necessarily, though max/min were), but don't think had any notes on them

@jorisvandenbossche
Copy link
Member

What was actually added? (I just realize I don't fully understand this)

For example max() already is an index method in 0.13, so I don't fully get what support 'max()/... on Index types' in the release notes is supposed to mean?

@jreback
Copy link
Contributor Author

jreback commented May 22, 2014

all of these were added as mixin methods that is they were moved from index/series directly to the ops mixin class (so max.min) were not added to index

but the other were added to index

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants