-
-
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
CLN: remove Index.__inv__ , add Index.__invert__ #22336
Conversation
test_arithmetic is ok, pls target 0.24 |
In general, if something is not a (critical) bug fix, it should never be considered for a bug fix release (and certainly not something that is more an API clean-up) |
Speaking of which, shouldn't we rather deprecate this first before removing? |
OK, didn't yet read your issue, so missed that |
8edd118
to
975e634
Compare
Codecov Report
@@ Coverage Diff @@
## master #22336 +/- ##
=========================================
Coverage ? 92.08%
=========================================
Files ? 169
Lines ? 50704
Branches ? 0
=========================================
Hits ? 46689
Misses ? 4015
Partials ? 0
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #22336 +/- ##
=======================================
Coverage 92.05% 92.05%
=======================================
Files 169 169
Lines 50709 50709
=======================================
Hits 46679 46679
Misses 4030 4030
Continue to review full report at Codecov.
|
975e634
to
72ebc5d
Compare
@jreback @jorisvandenbossche while I was at it, given that this is going to 0.24 anyway, I thought it wouldn't harm to actually enable |
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 also check that the ops are correctly defined in pandas/core/arrays/base.py (for EA arrays).
|
||
class TestNumericOps(object): | ||
|
||
@pytest.mark.parametrize('opname', ['neg', 'pos', 'abs', 'inv', 'invert']) |
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 add this in conftest as a fixture (unary_op_fixtures)?
|
||
if opname == 'inv': | ||
# GH 22335 - 'inv' was wrong and was removed | ||
pytest.raises(AttributeError, call_method) |
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.
make these if/elseif, then don't need the return (put the final clause in the else)
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.
Apart from Jeff's comments, looks good
@toobaz if you can rebase and check extension arrays |
@jreback I'm afraid I can do that only in September, but while I'm at it:
In the general
Do you mean adding EA arrays to (In any case, if some tests fail I would defer to another PR) |
closing as stale. if you want to continue, pls merge master and re-open. |
git diff upstream/master -u -- "*.py" | flake8 --diff
@jreback @jorisvandenbossche suggestions on where to put a test (
pandas/tests/test_arithmetic.py
would seem the right place, except it only contains tests involving datetimelike) and which version to target (not sure 0.23.5 is OK)?