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

REGR: disallow mean of period column again #33758

Merged
merged 2 commits into from
Apr 24, 2020

Conversation

jorisvandenbossche
Copy link
Member

This reverts parts of #32426 and #29941 (those are only on master, not yet released), to disallow taking the mean on a Period dtype column again. The mean for period is not supported on PeriodArray itself or for a Series of period dtype (on purpose, see discussion in #24757 when mean for datetimelike was added), so for DataFrame columns it should also not work.

See also comment at #32426 (comment)

cc @jbrockmendel

@jorisvandenbossche jorisvandenbossche added Numeric Operations Arithmetic, Comparison, and Logical operations Datetime Datetime data dtype labels Apr 24, 2020
@jorisvandenbossche jorisvandenbossche added this to the 1.1 milestone Apr 24, 2020
@jbrockmendel
Copy link
Member

Does this also disable median for PeriodDtype?

@jorisvandenbossche
Copy link
Member Author

Does this also disable median for PeriodDtype?

Hmm, yes, indeed.

Now, I would say that can be left out of this PR, because:

So properly supporting median basically needs PeriodArray._reduce to support median, and while this is certainly something we should do, my feeling is that this is better done in a dedicated PR for that (with appropriate testing).

@jorisvandenbossche
Copy link
Member Author

@jbrockmendel does that sound OK to you?
(and other comments on the code changes itself?)

@jbrockmendel
Copy link
Member

does that sound OK to you?

Yes, this is a Worth Doing It Right situation.

@jreback jreback merged commit 68132f3 into pandas-dev:master Apr 24, 2020
@jreback
Copy link
Contributor

jreback commented Apr 24, 2020

lgtm. this does not test / disable median?

@jorisvandenbossche jorisvandenbossche deleted the period-mean-disable branch April 25, 2020 07:53
@jorisvandenbossche
Copy link
Member Author

See my answer to Brock above

@jreback
Copy link
Contributor

jreback commented Apr 25, 2020

kk great @jorisvandenbossche

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Numeric Operations Arithmetic, Comparison, and Logical operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants