Skip to content
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

BUG: MultiIndex.drop does not raise if labels are partially found #37830

Merged
merged 11 commits into from
Nov 26, 2020
Merged

BUG: MultiIndex.drop does not raise if labels are partially found #37830

merged 11 commits into from
Nov 26, 2020

Conversation

GYHHAHA
Copy link
Contributor

@GYHHAHA GYHHAHA commented Nov 14, 2020

@pep8speaks
Copy link

pep8speaks commented Nov 14, 2020

Hello @GYHHAHA! 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-11-26 00:35:30 UTC

@@ -2157,9 +2157,10 @@ def _drop_from_level(self, codes, level, errors="raise"):
index = self.levels[i]
values = index.get_indexer(codes)

not_found = np.array(codes)[np.array(values) == -1].tolist()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work after #37794 is merged. Could you rebase afterwards? Please check for len(not_found)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the to_list necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary, I will alter this after phofl's PR merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could rebase now

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will review after rebase

@GYHHAHA
Copy link
Contributor Author

GYHHAHA commented Nov 19, 2020

cc @jreback

@GYHHAHA GYHHAHA requested a review from jreback November 22, 2020 07:35
@jreback jreback added this to the 1.2 milestone Nov 25, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, very minor comment, can you merge master and ping on green.

cc @phofl @jbrockmendel if comments

pandas/tests/indexes/multi/test_drop.py Show resolved Hide resolved
Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, minor comments

@@ -147,3 +147,22 @@ def test_drop_with_nan_in_index(nulls_fixture):
msg = r"labels \[Timestamp\('2001-01-01 00:00:00'\)\] not found in level"
with pytest.raises(KeyError, match=msg):
mi.drop(pd.Timestamp("2001"), level="date")


def test_single_level_drop():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename to something like test_single_level_drop_partially_missing_elements?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GYHHAHA can you do this one, ping on green.

pandas/tests/indexes/multi/test_drop.py Show resolved Hide resolved
@jreback jreback merged commit f15c4bc into pandas-dev:master Nov 26, 2020
@jreback
Copy link
Contributor

jreback commented Nov 26, 2020

thanks @GYHHAHA very nice, keep em coming!

@GYHHAHA GYHHAHA deleted the patch-1 branch November 27, 2020 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: MultiIndex.drop does not raise if labels are partially found
5 participants