pandas
pandas copied to clipboard
REGR: Ignore eval inplace
- [x] closes #47449
- [x] Tests added and passed if fixing a bug or adding a new feature
- [x] All code checks passed.
- [x] Added type annotations to new arguments/methods/functions.
- [x] Added an entry in the latest
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.
The bad commit that causes this issue (see https://github.com/pandas-dev/pandas/pull/43406) is the rootcause for multiple others, so am not sure if we want to tackle the overall problem bit by bit or if there is a generic solution to all of them.
Hello @CloseChoice! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:
Comment last updated at 2022-08-19 08:09:31 UTC
Could you add a whatsnew? Is there a suitable class for this test?
The root cause was intended, so we have to fix them where they occur.
Could you add a whatsnew? Is there a suitable class for this test?
The root cause was intended, so we have to fix them where they occur.
Thanks for the answer. I added a whatsnew entry.
@CloseChoice I started converting the suggestion at https://github.com/pandas-dev/pandas/issues/47449#issuecomment-1180701954 into a PR, missing that you already opened one with a very similar fix! I hope you don't mind, but so I merged some of the changes I started doing in your PR:
- Also checking for
inplacein eval.py (when not doing inplace, I think thetarget[assigner] = retwithout loc is faster), as suggested in https://github.com/pandas-dev/pandas/issues/47449#issuecomment-1180701954). In addition I also replaced (Series, DataFrame) with NDFrame - I slightly updated the test (used some simpler (less) values), and also added a check for a viewing Series to be updated (in addition to viewing DataFrame)
Is it possible to get there with a MultiIndex? Similar to https://github.com/pandas-dev/pandas/pull/47774 where the dimension reduction bothers us
I am not sure if eval supports referring (or assigning) to MultiIndex columns (I also don't directly see a test in test_eval.py that involved MultiIndex). I tried a few things that all fail:
# those two give syntax error when parsing the ast
df.eval("('A', 'a') = ('A', 'b') + ('B', 'a')")
df.eval("\"('A', 'a')\" = \"('A', 'b')\" + \"('B', 'a')\"")
# those two each give another error
df.eval("D = ('A', 'b') + ('B', 'a')")
df.eval("D = \"('A', 'b')\" + \"('B', 'a')\"")
If not using existing column names, but just assigning a new column, that still seems to work with this PR:
In [1]: df = DataFrame({("A", "a"): [1, 2, 3], ("A", "b"): [4, 5, 6], ("B", "a"): [7, 8, 9]})
In [2]: df
Out[2]:
A B
a b a
0 1 4 7
1 2 5 8
2 3 6 9
In [3]: df.eval("D = 1")
Out[3]:
A B D
a b a
0 1 4 7 1
1 2 5 8 1
2 3 6 9 1
In [4]: df.eval("E = 1", inplace=True)
In [5]: df
Out[5]:
A B E
a b a
0 1 4 7 1
1 2 5 8 1
2 3 6 9 1
Thanks for checking
@jorisvandenbossche I don't mind at all. Thanks for picking this up and improving upon it.
Greenish, merging
thx @CloseChoice
Owee, I'm MrMeeseeks, Look at me.
There seem to be a conflict, please backport manually. Here are approximate instructions:
- Checkout backport branch and update it.
git checkout 1.4.x
git pull
- Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 838b04f09fd1cbac2e2ff9307941da6f47c8b792
- You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #47550: REGR: fix eval with inplace=True to correctly update column values inplace'
- Push to a named branch:
git push YOURFORK 1.4.x:auto-backport-of-pr-47550-on-1.4.x
- Create a PR against branch 1.4.x, I would have named this PR:
"Backport PR #47550 on branch 1.4.x (REGR: fix eval with inplace=True to correctly update column values inplace)"
And apply the correct labels and milestones.
Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!
Remember to remove the Still Needs Manual Backport label once the PR gets merged.
If these instructions are inaccurate, feel free to suggest an improvement.