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.

Sign up for GitHub

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

Jump to bottom

CLN: Removed overridden Timedelta properties (#18242) #18374

Merged
merged 1 commit into from Nov 20, 2017
Merged

CLN: Removed overridden Timedelta properties (#18242) #18374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v0.22.0.txt
View file
Open in desktop
Expand Up @@ -77,7 +77,7 @@ Performance Improvements
- Added a keyword argument, ``cache``, to :func:`to_datetime` that improved the performance of converting duplicate datetime arguments (:issue:`11665`)
- :class`DateOffset` arithmetic performance is improved (:issue:`18218`)
- Converting a ``Series`` of ``Timedelta`` objects to days, seconds, etc... sped up through vectorization of underlying methods (:issue:`18092`)
-
- The overriden ``Timedelta`` properties of days, seconds and microseconds have been removed, leveraging their built-in Python versions instead (:issue:`18242`)

.. _whatsnew_0220.docs:

Expand Down
30 changes: 0 additions & 30 deletions pandas/_libs/tslibs/timedeltas.pyx
View file
Open in desktop
Expand Up @@ -665,36 +665,6 @@ cdef class _Timedelta(timedelta):
else:
return "D"

@property
def days(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

technically then won't self._sign be wrong (not that we are actually using it here)? I guess this is ok

Copy link
Member Author

@WillAyd WillAyd Nov 19, 2017 •
edited

Choose a reason for hiding this comment

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

The only place where self._sign gets used is in _repr_base, which calls _ensure_components at the start of the method to set self._sign. That happens irrespective of the property definitions.

If _repr_base were ever re-factored in the future it could definitely be missed. OK to open a separate issue to see if we can refactor the _repr_base call and make it more "future-proof" by removing the need for self._sign altogether?

FWIW I checked the test cases and didn't see one that explicitly handles the repr of negative Timedeltas, so I'll add one and re-push

Copy link
Member Author

Choose a reason for hiding this comment

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

Scratch my comment on not having test cases - you'll see some in test_arithmetic that ensure we have the appropriate Timedelta repr, so I'm not planning to re-push unless you have additional changes you'd like.

pandas/pandas/tests/indexes/timedeltas/test_arithmetic.py

Line 235 in a172ff9

expected = TimedeltaIndex(['0 days', '-1 days', '-2 days'], name='bar')

"""
Number of Days

.components will return the shown components
"""
self._ensure_components()
return self._d

@property
def seconds(self):
"""
Number of seconds (>= 0 and less than 1 day).

.components will return the shown components
"""
self._ensure_components()
return self._seconds

@property
def microseconds(self):
"""
Number of microseconds (>= 0 and less than 1 second).

.components will return the shown components
"""
self._ensure_components()
return self._microseconds

@property
def nanoseconds(self):
"""
Expand Down