-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
ENH: support datetime64, datetime64tz in nanops.mean, nanops.median #29941
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine, can you add a deprecation note
pandas/core/frame.py
Outdated
# e.g. in nanops trying to convert strs to float | ||
# TODO: the ValueError is raised in trying to convert str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this should be a TypeError, where is the ValueError coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is trying to convert to str?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you still need this ValueError? (as i think you changed below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll re-run without catching ValueError. IIRC it was in nanops._ensure_numeric
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah, there are 3 tests where x.astype(np.float64)
raises a ValueError in _ensure_numeric with a object-dtype x
that contains a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just removed this and re-raised as TypeError on nanops 1292-1296
pandas/core/frame.py
Outdated
# e.g. in nanops trying to convert strs to float | ||
# TODO: the ValueError is raised in trying to convert str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is trying to convert to str?
pandas/core/frame.py
Outdated
# e.g. in nanops trying to convert strs to float | ||
# TODO: the ValueError is raised in trying to convert str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you still need this ValueError? (as i think you changed below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comments, but can merge if blocking
this looks good. can you add a deprecation note in whatsnew and rebase. |
whatsnew added, rebased+green |
lgtm. once more rebase and merge on green. (also add to the deprecated issue tracker as well). |
Thanks @jbrockmendel |
Tracking down relevant issues and figuring out test coverage is the "WIP" part of this.