BUG: Multiindexed series .at fix #32520

rhshadrach · 2020-03-07T17:00:57Z

closes Series with MultiIndex "at" function raises "TypeError" #26989
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pandas/core/indexing.py

pandas/tests/series/indexing/test_indexing.py

pandas/tests/indexing/test_scalar.py

jbrockmendel · 2020-03-13T16:47:18Z

pandas/tests/indexing/test_scalar.py

+def test_multiindex_series_loc_set():
+    series = pd.Series([1, 2], index=[[1, 2], [3, 4]])
+    series.loc[1, 3] = 3
+    assert series.loc[1, 3] == 3


i dont think the loc tests are needed, unless that behavior is being fixed

I believe @jreback requested loc to be added because the rest of the tests found in test_scalar test both at and loc.

jreback

tests comments, ping on green.

pandas/tests/indexing/test_scalar.py

jreback

pls add a whatsnew note in 1.1, bugfixes for indexing

rhshadrach · 2020-03-14T16:35:51Z

@jreback ping

jbrockmendel · 2020-03-18T18:56:06Z

@rhshadrach can you rebase and ill take another look

pep8speaks · 2020-03-18T20:12:35Z

Hello @rhshadrach! 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! 🍻

Comment last updated at 2020-05-18 20:00:11 UTC

rhshadrach · 2020-03-18T21:19:30Z

@jbrockmendel I've rebased and all checks pass.

jreback · 2020-03-19T00:18:31Z

lgtm. can you merge master. cc @jbrockmendel merge when happy.

jreback · 2020-03-26T00:55:47Z

lgtm. cc @jbrockmendel merge when ready.

pandas/core/indexing.py

pandas/tests/indexing/test_scalar.py

jbrockmendel · 2020-03-26T22:36:18Z

A few comments, generally looks very good

rhshadrach · 2020-03-27T19:50:50Z

@jbrockmendel Thanks for the comments. I've addressed all of them. I believe failing checks are unrelated, but I'm having a hard time understanding the error messages.

jbrockmendel · 2020-03-27T21:17:17Z

LGTM.

Yes, the azure failure is unrelated. To be on the safe side we usually try to sort these out before merging anything new.

jorisvandenbossche

I just opened #33153 for a general API question that also impacts this PR.

So the fix to allow .at indexing for a Series with MultiIndex is certainly a welcome fix, but I only question that it should also work if not all levels of the MultiIndex are specified (a behaviour that this PR also enables).

jorisvandenbossche · 2020-03-30T19:24:46Z

pandas/tests/indexing/test_scalar.py

+    assert series.index.nlevels == 2
+    assert series.at[1, 3] == 1
+    assert series.loc[1, 3] == 1
+


Can you add a test for not providing all MultiIndex levels? (so eg series.at[1]).
And also a test with a duplicated label?

And maybe also an equivalent test for a DataFrame (it seems to work, but would be good to ensure we also have an explicit test for that)

Sure - I have it mostly done, but ran into a question. For the series

series = pd.Series([1, 2, 3], index=[[1, 2, 2], [3, 4, 4]])

i.e.

1 3 1 2 4 2 4 3 dtype: int64

what should be the output of series.at[1, 3]? I was expecting to just get the scalar 1, but currently I'm getting

1 3 1 dtype: int64

After thinking about this - perhaps what we have currently is the desired output. My reason for this is that the shape doesn't change depending on the input - (1, 3) vs (2, 4).

IMO, that should be a scalar (but that is also the reason that I asked to test it, to be sure this is covered ànd to discuss the semantics we want)

jorisvandenbossche · 2020-03-31T06:06:51Z

Seems to me #26989 does not address using .at when not all levels are specified. Perhaps you meant another issue?

It was not described in that issue, but this PR is still enabling it:

In [5]: s = pd.Series(range(4), index=pd.MultiIndex.from_product([['a', 'b'], [1, 2]]))    

In [6]: s  
Out[6]: 
a  1    0
   2    1
b  1    2
   2    3
dtype: int64

In [7]: s.at['a']    
Out[7]: 
1    0
2    1
dtype: int64

This fails on 0.25.3 and 1.0.3

jorisvandenbossche · 2020-03-31T06:10:44Z

Ah, but it seems this also already works on master, so it has been "accidentally" enabled due to some other refactoring, I suppose. So OK, indeed not directly related to your PR.

rhshadrach · 2020-03-31T10:21:01Z

Ah, but it seems this also already works on master, so it has been "accidentally" enabled due to some other refactoring, I suppose. So OK, indeed not directly related to your PR.

With this, I think we should leave as-is in regards to non-scalar results. These can be fixed or documented and then tested for after discussion in #33153. I added the tests for DataFrame though. Note that DataFrame.loc fails with an Index of tuples, so I did not add this test.

rhshadrach · 2020-04-01T21:40:55Z

@jorisvandenbossche changes made. Let me know if you agree with my assessment above.

jreback · 2020-04-06T23:49:37Z

@rhshadrach can you merge master. @jorisvandenbossche ok here?

rhshadrach · 2020-04-18T12:36:09Z

@jorisvandenbossche Let me know if there are any more changes you'd like.

jbrockmendel · 2020-04-22T22:57:04Z

@rhshadrach can you rebase

I think this may make #32855 unnecessary

rhshadrach · 2020-04-22T23:37:50Z

@jbrockmendel - done. Seems to me the only intersection with the PR you mentioned is the one test dealing with Series there. This PR only deals with Series, where as most of that is dealing with DataFrames.

rhshadrach · 2020-05-04T21:09:16Z

@jorisvandenbossche @jreback I think this PR is all set.

Addresses: GH26989

jreback

lgtm. a doc issue, ping when green.

doc/source/whatsnew/v1.1.0.rst

rhshadrach · 2020-05-18T21:17:31Z

@jreback Note added back, green

jreback · 2020-05-18T23:13:08Z

pandas/tests/indexing/test_scalar.py

+    series = df["a"]
+    assert series.index.nlevels == 2
+    series.at[1, 3] = 5
+    assert series.at[1, 3] == 5


is this the behavior now on master? IOW is this an api change

Current behavior on master is to raise ValueError for any of the at lines, behavior with the loc lines is the same as master. I would describe this as a bugfix and not an api change.

jreback · 2020-05-25T22:07:55Z

thanks @rhshadrach

rhshadrach changed the title ~~[BUG] Multiindex .at fix~~ [BUG] Multiindexed series .at fix Mar 7, 2020

This comment has been minimized.

Sign in to view

jbrockmendel reviewed Mar 7, 2020

View reviewed changes

pandas/core/indexing.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 9, 2020

View reviewed changes

pandas/core/indexing.py Outdated Show resolved Hide resolved

jreback added Indexing Related to indexing on series/frames, not to indexes themselves Bug MultiIndex labels Mar 11, 2020

jreback requested changes Mar 11, 2020

View reviewed changes

pandas/tests/series/indexing/test_indexing.py Outdated Show resolved Hide resolved

pandas/tests/series/indexing/test_indexing.py Outdated Show resolved Hide resolved

pandas/tests/series/indexing/test_indexing.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 13, 2020

View reviewed changes

pandas/tests/indexing/test_scalar.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 13, 2020

View reviewed changes

jreback requested changes Mar 14, 2020

View reviewed changes

pandas/tests/indexing/test_scalar.py Outdated Show resolved Hide resolved

pandas/tests/indexing/test_scalar.py Outdated Show resolved Hide resolved

jreback requested changes Mar 14, 2020

View reviewed changes

jreback added this to the 1.1 milestone Mar 14, 2020

This comment has been minimized.

Sign in to view

rhshadrach force-pushed the multiindex_at_fix branch from 689f52d to 26fbf7c Compare March 18, 2020 20:12

jreback approved these changes Mar 19, 2020

View reviewed changes

rhshadrach force-pushed the multiindex_at_fix branch from aeea52d to e3bc8b9 Compare March 25, 2020 22:17

jbrockmendel reviewed Mar 26, 2020

View reviewed changes

pandas/core/indexing.py Show resolved Hide resolved

jbrockmendel reviewed Mar 26, 2020

View reviewed changes

pandas/tests/indexing/test_scalar.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 26, 2020

View reviewed changes

pandas/tests/indexing/test_scalar.py Outdated Show resolved Hide resolved

rhshadrach force-pushed the multiindex_at_fix branch from e3bc8b9 to bee54e7 Compare March 27, 2020 19:06

jorisvandenbossche changed the title ~~[BUG] Multiindexed series .at fix~~ BUG: Multiindexed series .at fix Mar 30, 2020

jorisvandenbossche requested changes Mar 30, 2020

View reviewed changes

rhshadrach force-pushed the multiindex_at_fix branch from bee54e7 to 67b8150 Compare March 31, 2020 10:15

rhshadrach mentioned this pull request Apr 22, 2020

BUG: Improved error msg #32855

Closed

5 tasks

rhshadrach added 3 commits May 10, 2020 13:02

[BUG] fix .at for multiindexed series

083b1ec

Addresses: GH26989

Replaced isinstance check, fixed comments, added asserts in tests

88bca04

Added tests for DataFrame alongside Series

f325e09

rhshadrach force-pushed the multiindex_at_fix branch from 76f196a to f325e09 Compare May 10, 2020 17:11

rhshadrach requested a review from jorisvandenbossche May 16, 2020 13:36

jreback requested changes May 17, 2020

View reviewed changes

doc/source/whatsnew/v1.1.0.rst Show resolved Hide resolved

rhshadrach added 3 commits May 17, 2020 18:12

Reverted whatsnew line removal

d03b102

Merge remote-tracking branch 'upstream/master' into multiindex_at_fix

bf4f1c4

Merge remote-tracking branch 'upstream/master' into multiindex_at_fix

89ee128

jreback approved these changes May 18, 2020

View reviewed changes

jreback reviewed May 18, 2020

View reviewed changes

jreback merged commit 55e8891 into pandas-dev:master May 25, 2020

rhshadrach deleted the multiindex_at_fix branch July 11, 2020 16:02

BUG: Multiindexed series .at fix #32520

BUG: Multiindexed series .at fix #32520

Conversation

rhshadrach commented Mar 7, 2020 • edited Loading

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

rhshadrach commented Mar 14, 2020

This comment has been minimized.

This comment has been minimized.

jbrockmendel commented Mar 18, 2020

pep8speaks commented Mar 18, 2020 • edited Loading

Comment last updated at 2020-05-18 20:00:11 UTC

rhshadrach commented Mar 18, 2020

jreback commented Mar 19, 2020

jreback commented Mar 26, 2020

jbrockmendel commented Mar 26, 2020

rhshadrach commented Mar 27, 2020

jbrockmendel commented Mar 27, 2020

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Mar 31, 2020

jorisvandenbossche commented Mar 31, 2020

rhshadrach commented Mar 31, 2020

rhshadrach commented Apr 1, 2020

jreback commented Apr 6, 2020

rhshadrach commented Apr 18, 2020

jbrockmendel commented Apr 22, 2020

rhshadrach commented Apr 22, 2020

rhshadrach commented May 4, 2020 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

rhshadrach commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented May 25, 2020

rhshadrach commented Mar 7, 2020 •

edited

Loading

pep8speaks commented Mar 18, 2020 •

edited

Loading

rhshadrach commented May 4, 2020 •

edited

Loading