-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
Continue Collecting Arithmetic Tests #22267
Conversation
Hello @jbrockmendel! Thanks for updating the PR.
Comment last updated on August 09, 2018 at 22:34 Hours UTC |
def test_mul_int(self, idx): | ||
result = idx * 1 | ||
tm.assert_index_equal(result, idx) | ||
@pytest.mark.parametrize('op', [operator.mul, ops.rmul, operator.floordiv]) |
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.
future nit, might as well import the other operator
to ops
to make things consistent
else: | ||
aliases = {'div': 'truediv'} | ||
|
||
for op in ops: |
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.
woa needs parametrization big time :>
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.
Very much so.
@@ -1,12 +1,17 @@ | |||
# -*- coding: utf-8 -*- | |||
import operator |
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.
aren't you moving things to pandas/tests/arithmetic
?
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.
The focus is on tests that can be shared across Index/Series/DataFrame(/EA). flex
ops don't satisfy that criterion.
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.
In case unclear, the tests added to tests.frame.test_arithmetic were moved from tests.frame.test_operators; so this is a strictly better location.
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.
yeah its ok.
looks fine. ping on green. (go ahead and merge) |
Codecov Report
@@ Coverage Diff @@
## master #22267 +/- ##
=======================================
Coverage 92.08% 92.08%
=======================================
Files 169 169
Lines 50704 50704
=======================================
Hits 46691 46691
Misses 4013 4013
Continue to review full report at Codecov.
|
thanks |
A small amount of cleanup/parametrization, but mostly moving tests ~unchanged.
A few tests in tests.frame.test_operators are moved to tests.frame.test_arithmetic.
In a bunch of cases I'm trying to not move things that will cause merge conflicts with #22163; will get those in an upcoming pass.