-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
Performance of maybe_box_datetimelike #30520 #30531
Conversation
Hello @ivan-vasilev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-01-01 23:40:02 UTC |
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.
pls add an asv and a whatsnew note
i suppose could change for Timedelta as well
pandas/core/common.py
Outdated
@@ -84,9 +84,11 @@ def maybe_box(indexer, values, obj, key): | |||
def maybe_box_datetimelike(value): | |||
# turn a datetime like into a Timestamp/timedelta as needed | |||
|
|||
if isinstance(value, (np.datetime64, datetime)): | |||
if isinstance(value, (np.datetime64, datetime)) \ | |||
and not isinstance(value, tslibs.Timestamp): |
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.
can also exclude NaT
See my comment in the issue: #30520 (comment). Is this really the appropriate place for the performance fix? Or should this be done in the |
linting issue should be fixable by running |
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.
some comments, pls add a whatsnew (perf section), be generic about it. e.g. elaborate on what a user would see at a high level, e.g. you have a Series and do xyz to it, now its faster. also add this example as an asv.
@@ -121,4 +122,13 @@ def time_convert(self, data): | |||
lib.maybe_convert_numeric(data, set(), coerce_numeric=False) | |||
|
|||
|
|||
from .pandas_vb_common import setup # noqa: F401 isort:skip |
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.
you need to leave this
pandas/core/common.py
Outdated
and obj.stop is None | ||
and obj.step is None | ||
isinstance(obj, slice) | ||
and obj.start is None |
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.
can you avoid other whitespace changes
this is subsumed by #30676 (though if you still think there is a perf benefit after that certainly ping). |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
maybe_box_datetimelike
avoids unnecessary conversion ofvalue
totslibs.Timestamp
ortslibs.Timedelta
ifvalue
is already an instance oftslibs.Timestamp
ortslibs.Timedelta
.