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

ENH: implement Timedelta.__mod__ and __divmod__ #19755

Merged
merged 7 commits into from
Feb 19, 2018

Conversation

jbrockmendel
Copy link
Member

This implements exclusively the __mod__, __rmod__, __divmod__, __rdivmod__ parts of #19365, does not implement any of the bugfixes or tests that got rolled into that PR. Several tests are xfailed for now, will be fixed after #19738. These tests will be rebased/moved to test_arithmetic after #19752.

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@jorisvandenbossche jorisvandenbossche changed the title implement Timedelta.__mod__ etc, test but do _not_ fix related bugs ENH: implement Timedelta.__mod__ and __divmod__ Feb 19, 2018
@jorisvandenbossche jorisvandenbossche added the Timedelta Timedelta data type label Feb 19, 2018
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

small comment, looks good to me

@@ -571,6 +585,7 @@ Other API Changes
- Set operations (union, difference...) on :class:`IntervalIndex` with incompatible index types will now raise a ``TypeError`` rather than a ``ValueError`` (:issue:`19329`)
- :class:`DateOffset` objects render more simply, e.g. "<DateOffset: days=1>" instead of "<DateOffset: kwds={'days': 1}>" (:issue:`19403`)
- :func:`pandas.merge` provides a more informative error message when trying to merge on timezone-aware and timezone-naive columns (:issue:`15800`)
- :func:`Timedelta.__mod__`, :func:`Timedelta.__divmod__` now accept timedelta-like and numeric arguments instead of raising ``TypeError`` (:issue:`19365`)
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 the same as above? Isn't it enough to mention it once?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an attempt to follow the instructions in #19365, some of which were unclear. I'm open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion would be to just remove this one, but I don't know the reason for the previous instruction

Copy link
Member Author

Choose a reason for hiding this comment

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

@JRBACK is there consensus on this suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i guess I wasn't clear. if you make a sub-section then no reason to repeat things, so kill this entry as it dupes the sub-section.

@@ -254,6 +254,186 @@ def test_rfloordiv(self):
with pytest.raises(TypeError):
ser // td

def test_td_mod_timedeltalike(self):
Copy link
Member

Choose a reason for hiding this comment

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

Like test_rfloordiv above, I think you can leave out the _td_ in all test names (the class name already makes clear it is about Timedelta)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@codecov
Copy link

codecov bot commented Feb 19, 2018

Codecov Report

Merging #19755 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #19755   +/-   ##
=======================================
  Coverage    91.6%    91.6%           
=======================================
  Files         150      150           
  Lines       48864    48864           
=======================================
  Hits        44763    44763           
  Misses       4101     4101
Flag Coverage Δ
#multiple 89.98% <ø> (ø) ⬆️
#single 41.77% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a76e5b4...aefa6d6. Read the comment docs.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

doc otherwise lgtm (mod suggestion below :)

ping on green.

# divmod against a timedelta-like returns a pair (int, Timedelta)
divmod(datetime.timedelta(hours=2), pd.Timedelta(minutes=11))

# divmod against a numeric returns a pair (Timedelta, Timedelta)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@@ -571,6 +585,7 @@ Other API Changes
- Set operations (union, difference...) on :class:`IntervalIndex` with incompatible index types will now raise a ``TypeError`` rather than a ``ValueError`` (:issue:`19329`)
- :class:`DateOffset` objects render more simply, e.g. "<DateOffset: days=1>" instead of "<DateOffset: kwds={'days': 1}>" (:issue:`19403`)
- :func:`pandas.merge` provides a more informative error message when trying to merge on timezone-aware and timezone-naive columns (:issue:`15800`)
- :func:`Timedelta.__mod__`, :func:`Timedelta.__divmod__` now accept timedelta-like and numeric arguments instead of raising ``TypeError`` (:issue:`19365`)
Copy link
Contributor

Choose a reason for hiding this comment

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

oh i guess I wasn't clear. if you make a sub-section then no reason to repeat things, so kill this entry as it dupes the sub-section.

td = Timedelta(days=2, hours=6)

result = divmod(td, 53 * 3600 * 1e9)
assert result[0] == Timedelta(1, unit='ns')
Copy link
Contributor

Choose a reason for hiding this comment

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

slightly OT: we have test_nat.py so either should add this there, or have here, or maybe both.

@jreback jreback added this to the 0.23.0 milestone Feb 19, 2018
@jreback
Copy link
Contributor

jreback commented Feb 19, 2018

lgtm ping on green.

@jbrockmendel
Copy link
Member Author

Ping

@jreback
Copy link
Contributor

jreback commented Feb 19, 2018

thanks!

@jreback jreback merged commit e0f6bea into pandas-dev:master Feb 19, 2018
harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018
@jbrockmendel jbrockmendel deleted the tdmod2 branch June 22, 2018 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants