BUG: wrap __idiv__ to avoid making a copy (#12962) #17589
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17589 +/- ##
==========================================
- Coverage 91.26% 91.24% -0.02%
==========================================
Files 163 163
Lines 49806 49808 +2
==========================================
- Hits 45455 45448 -7
- Misses 4351 4360 +9
Continue to review full report at Codecov.
|
gfyoung
added
Bug
Dtype Conversions
pandas/tests/frame/test_operators.py
Outdated
| assert id(df) == expected | ||
| df *= 2. | ||
| assert id(df) == expected | ||
| df /= 2. |
There was a problem hiding this comment.
this fails before your change?
There was a problem hiding this comment.
yes, it will only fail for python 2.7 using classic division (which people shouldn't be using but the behavior is still unexpected)
you can verify by doing
git checkout master pandas/core/ops.py ; pytest pandas/tests/frame/test_operators.py
since the fix and test are in one commit.
pandas/tests/frame/test_operators.py
Outdated
| assert id(df) == expected | ||
| df /= 2. | ||
| assert id(df) == expected | ||
| df **= 2. |
There was a problem hiding this comment.
I added more missing inplace operators (ifloordiv, imod, iand, ior, ixor). The only ones excluded now are iconcat, ilshift and irshift which I believe are not supported.
| @@ -186,8 +186,10 @@ def add_special_arithmetic_methods(cls, arith_method=None, | |||
| arith_method : function (optional) | |||
There was a problem hiding this comment.
it does not make sense for arith_method to be an optional argument because _create_methods will try to call it without checking if it is None
pandas/tests/frame/test_operators.py
Outdated
| # no id change | ||
| df = DataFrame({'a': [1., 2., 3.]}) | ||
| expected = id(df) | ||
| df += 2. |
There was a problem hiding this comment.
can you make this a new test, ideally you can parametrize this and also test (in addition to identity) equality with the non-inplace op
e.g. something like
@pytest.mark.parmatrize('op', ['add', 'sub'......])
def test_inplace_ops_identity2(self, op):
df = DataFrame(...)
df_copy = df.copy()
iop = '__i{}__'.format(op)
op = '__{}__'.format(op)
getattr(df, iop)(2.)
expected = getattr(df_copy, op)(2.)
tm.assert_frame_equal(df, expected)
....check id here too
|
Hello @dkamm! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on September 24, 2017 at 22:10 Hours UTC |
|
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff