fixes #26622: pd.MultiIndex.isin should fail when input args are too short #26623
Conversation
|
pls always add tests, how would u know if it works otherwise? |
|
@jreback Already on it! I'll push em in a sec. What does the |
simonjayhawkins
added
Error Reporting
doc/source/whatsnew/v0.25.0.rst |
Codecov Report
@@ Coverage Diff @@
## master #26623 +/- ##
==========================================
+ Coverage 91.86% 91.87% +<.01%
==========================================
Files 174 174
Lines 50722 50696 -26
==========================================
- Hits 46594 46575 -19
+ Misses 4128 4121 -7
Continue to review full report at Codecov.
|
jreback
changed the title
pd.MultiIndex.isin fail when input args are too short
|
Great. Thanks @simonjayhawkins for taking the time to review this. |
| @@ -3175,6 +3175,14 @@ def _wrap_joined_index(self, joined, other): | |||
| @Appender(Index.isin.__doc__) | |||
| def isin(self, values, level=None): | |||
| if level is None: | |||
| # validate value length | |||
There was a problem hiding this comment.
hmm, we already do this sort of validation in _verify_integrity in the MI construction, so I think the MI is actually getting constructed correctly, but you have nan's in the levels.
I am not opposed to the check, but it seems that maybe the semantics of handling this are not exactly right, meaning what you passed in your tests is actually valid.
|
cc @toobaz for thoughts on this |
| @@ -608,6 +608,7 @@ MultiIndex | |||
| ^^^^^^^^^^ | |||
|
|
|||
| - Bug in which incorrect exception raised by :class:`Timedelta` when testing the membership of :class:`MultiIndex` (:issue:`24570`) | |||
| - :meth:`MultiIndex.isin` now raises ``ValueError`` when items in values are not the same length as the number of levels in the MultiIndex (:issue:`26622`) | |||
|
can you merge master and update to comments |
|
can you merge master and move the note to 1.0.0 |
|
Closing as this looks stale, but @telamonian ping if you'd like to pick back up |
git diff upstream/master -u -- "*.py" | flake8 --diffThis fixes #26622 by making
pd.MultiIndex.isinfail when input args are too short, as well as when they are too long (the current behavior). It also replaces existing error message with something a little bit clearly and more directly related toisinand its parameters.Currently,
isindoes some validation by constructing a newMultiIndexand then taking itsvalues:Though the code I wrote should handle all validation relating to the length of the elements of
values, I left this line in since I was unsure if it was validating/fixing anything else. Does anyone know if that line is safe to remove now?