ENH: Added index to testing assert message when series values differ #31435
Conversation
|
@WillAyd : If the user calls assert_frame_equal with check_like=False (the default) then the indexs must contain the same values in the same order, so they will be identical. Hence no need to specify which one is being printed. If the user calls assert_frame_equal with check_like=True, then the indexes must contain the same values, but the order may differ. However, the field values for each index (row) must match for the frames to be considdered equal. The (existing) code reindexes one of the frames in the order of the other and then compares them. Presuming the indexes have the same values but were ordered differently then one of the frames will have its rows re-ordered. This was previously confusing in the output, as there was no indication of which rows had moved or of their new order. This PR addresses that by adding the index. Because both frames must have the same values in their index (or the code wouldn't have got as far as checking the row values), it doesn't matter which index is being used, since the row values in each frame would have had the same index value. I hope that makes sense; I'm not sure how well I explained it there. And thanks for looking at the PR :-) |
|
Looks like the build failure was spurious. Will hopefully vanish when I commit changes to address comments above. |
|
@WillAyd Does the description above make sense? |
|
@WillAyd Sorry if you wanted me to make a change, but I think I have addressed your comments. GitHub still thinks there is a change requested though, and I can't figure out what I need to do to resolve it. |
|
So IIUC this only applies to |
|
@WillAyd Yes, this change is specifically intended to address an issue with index reordering if check_like=True. The extra output (the index values) is also generated when check_like=False because it is equally valid and it keeps the output consistent, but when chec_like=False it doesn't add all that much, as the user would already know the index order. That said, the index values may make the output easier to refer to if the index is unordered or hard to remember. |
pandas/_testing.py
Outdated
| @@ -917,9 +924,10 @@ def raise_assert_detail(obj, message, left, right, diff=None): | |||
| elif is_categorical_dtype(right): | |||
| right = repr(right) | |||
|
|
|||
| msg = f"""{obj} are different | |||
| if isinstance(index, np.ndarray): | |||
There was a problem hiding this comment.
Oops - nothing.
Left over after refactor and should have been removed. Will remove. Thanks.
amilbourne
force-pushed
the
diff-with-index
branch
from
6231c8c to
114f88e
Compare
|
Hello @amilbourne! 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-03-21 22:09:52 UTC |
|
I just rebased this change against the latest in master to resolve the merge conflict and keep things clean. Sadly I missed a Pep 8 issue because the rebase didn't invoke the commit hooks :-/ |
|
@jreback I think I have addressed all your comments but I am unsure of the etiquette here. Should I click on the 'resolve conversation' button? 2 of your comments are probably resolved (presuming my changes were what you wanted) but 1 was a question and I am unsure whether my answer is sattisfactory. |
|
lgtm. @WillAyd @jorisvandenbossche @jbrockmendel if any comments. |
|
thanks @amilbourne nice addition! |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffThis implements the suggestion made in #31190
In short, where the values in 2 series differ, the assert exception message includes the index as well as the values. This helps when using assert_frame_equal with check_like=True, since the index may be reordered, making the output hard to understand.