Fix obj arguments in assertions #59460
Conversation
serl
force-pushed
the
testing-obj-minor-fixes
branch
from
06a5393 to
8cfbede
Compare
rhshadrach
added
Enhancement
Testing
|
Hello @rhshadrach, thanks for your review! |
There was a problem hiding this comment.
I added a little test case and I'm quite satisfied with the results. @rhshadrach @mroeschke what are the next steps? 🚀
| @@ -314,7 +314,7 @@ def _check_types(left, right, obj: str = "Index") -> None: | |||
| obj=lobj, | |||
| ) | |||
| # get_level_values may change dtype | |||
| _check_types(left.levels[level], right.levels[level], obj=obj) | |||
| _check_types(left.levels[level], right.levels[level], obj=lobj) | |||
There was a problem hiding this comment.
Didn't manage to write a test for this (type seems to be already checked just above), which, given its comment, smells very corner-case-y. Probably it's better to stop the head-banging, unless you have ideas on a test case, of course
serl
force-pushed
the
testing-obj-minor-fixes
branch
from
3ba3cd0 to
4c0c893
Compare
|
|
||
| MultiIndex level \\[1\\] values are different \\(25\\.0 %\\) | ||
| Index level \\[1\\] values are different \\(25\\.0 %\\) |
There was a problem hiding this comment.
I'm not sure I really like this change. I think MultiIndex level is clearer here
There was a problem hiding this comment.
I see. Just to be on the same page, have you got the opportunity to skim #59440?
There was a problem hiding this comment.
Ok so I tried to make a compromise (09a1ae5), which in short is:
root_obj = "MultiIndex" if obj == "Index" else obj
...
lobj = f"{root_obj} level [{level}]"I find that a little brittle, as it is based on the fact that about a hundred lines before there is obj: str = "Index".
...So I tried harder (ad99297), which correspond to the current state of the PR. That is having the obj in the assert_index_equal function definition being None by default and branching immediately:
if obj is None:
obj = "MultiIndex" if isinstance(left, MultiIndex) else "Index"Thoughts? Better ideas?
There was a problem hiding this comment.
Hello @mroeschke, have you got the chance to check this? Thanks!
There was a problem hiding this comment.
I'm not sure I really like this change. I think
MultiIndex levelis clearer here
No disagreement that it is slightly clearer, but it bucks the otherwise consistent behavior of assert_.*_equal utilizing obj when passed.
|
Thanks @serl |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.