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

Fix TimedeltaIndex +/- offset array #19095

Merged
merged 4 commits into from
Jan 7, 2018

Conversation

jbrockmendel
Copy link
Member

No issue specific to this, but analogus to #18849. There is a checkbox for this in #18824.

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

@jbrockmendel
Copy link
Member Author

After this we'll be able to get rid of _TimeOp entirely

name=tdi.name, freq='infer')

with tm.assert_produces_warning(PerformanceWarning):
res = tdi + other
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you be using the box here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

with tm.assert_produces_warning(PerformanceWarning):
tdi + anchored
with pytest.raises(TypeError):
with tm.assert_produces_warning(PerformanceWarning):
Copy link
Contributor

Choose a reason for hiding this comment

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

some duplication here

@jreback jreback added Frequency DateOffsets Timedelta Timedelta data type labels Jan 5, 2018
with pytest.raises(TypeError):
with tm.assert_produces_warning(PerformanceWarning):
anchored + tdi

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally could parameterize some of these for index, ndarray & Series as well (you can leave them here is ok). I believe you have some duplication in test_base

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, part of it is tricky because parametrizing with box and names at the same time is cumbersome. My thought at the moment is to get the bugs fixed and tested first and take on refactoring later (when there's not a constant flow of rebasing to be done).

@codecov
Copy link

codecov bot commented Jan 5, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@856c92b). Click here to learn what that means.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #19095   +/-   ##
=========================================
  Coverage          ?   91.51%           
=========================================
  Files             ?      148           
  Lines             ?    48778           
  Branches          ?        0           
=========================================
  Hits              ?    44639           
  Misses            ?     4139           
  Partials          ?        0
Flag Coverage Δ
#multiple 89.88% <88.23%> (?)
#single 41.61% <14.7%> (?)
Impacted Files Coverage Δ
pandas/core/indexes/datetimelike.py 97.08% <100%> (ø)
pandas/core/indexes/timedeltas.py 90.58% <84.61%> (ø)

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 856c92b...b9e9fff. Read the comment docs.

@@ -25,26 +28,88 @@ def freq(request):
return request.param


def _raises_and_warns(op, left, right):
Copy link
Contributor

Choose a reason for hiding this comment

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

name things properly if you must do this: assert_raises_type_error_and_performance_warning.

yes its pretty long but its readable. remember folks who have no history with this code will be reading it and have no idea unless its spelled our.

@jbrockmendel
Copy link
Member Author

Travis error is a timeout; should I re-push?

@jreback
Copy link
Contributor

jreback commented Jan 6, 2018

i restarted

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.

test comment, otherwise lgtm. ping when green.

expected_add = Series([tdi[n] + other[n] for n in range(len(tdi))],
name=names[2])

with tm.assert_produces_warning(PerformanceWarning):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put a comment on before these sub-section of tests, hard to follow your flow others, IOW enumerate the cases. only looking for a comment at lines 89, 100, 107

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

@jreback jreback added this to the 0.23.0 milestone Jan 6, 2018
@jbrockmendel
Copy link
Member Author

Http error on appveyor

@jbrockmendel
Copy link
Member Author

Ping

@jreback jreback merged commit e27ae70 into pandas-dev:master Jan 7, 2018
@jreback
Copy link
Contributor

jreback commented Jan 7, 2018

thanks!

@jbrockmendel jbrockmendel deleted the tdi_offsets branch January 7, 2018 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frequency DateOffsets Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants