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

ENH: Fix output of assert_frame_equal if indexes differ and check_like=True #37479

Merged
merged 4 commits into from
Nov 2, 2020

Conversation

amilbourne
Copy link
Contributor

@amilbourne amilbourne commented Oct 29, 2020

As described in #37478 This PR fixes some very misleading output when assert_frame_equal is called with differing index values and check_like=True

I have added a check_order parameter to the assert_index_equal function. This does essentially the same thing as the check_like parmeter on assert_frame_equal (but for indexes). I think check_order gives a much clearer idea of what the parameter does, but it is a break from the previous naming. I am unsure whether I should have stuck with check_like for the new parameter.

Copy link
Member

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

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

Thanks @amilbourne for the PR! Looks good

I don't know what the pandas policy is on API changes for the testing module (I think it's private but I think it gets used outside our testing suite) so one thing you might need to get agreement on

Related to the above if we regard this as a user-facing change you will need to add a whatsnew note

@ivanovmg
Copy link
Member

Thanks @amilbourne for the PR! Looks good

I don't know what the pandas policy is on API changes for the testing module (I think it's private but I think it gets used outside our testing suite) so one thing you might need to get agreement on

Related to the above if we regard this as a user-facing change you will need to add a whatsnew note

I guess that assert_index_equal is indeed a part of public API, since the docs are available here #37479 (review)

Comment on lines +670 to 671
check_order: bool = True,
rtol: float = 1.0e-5,
Copy link
Member

Choose a reason for hiding this comment

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

Probably it is necessary to have the new parameter added to the very end of this function signature in order not to break any user's code, which for some reason use positional arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

I debated that and it looked like the rtol and atol params had been added before the (older) obj param, so I decided to put check_order with the other check params. That said, obj is not something that most user code would pass so perhaps the comparrison doesn't hold.

Happy to move the parameter.

I also just noticed that I forgot to add a versionadded tag (will do).

@amilbourne
Copy link
Contributor Author

@ivanovmg, @arw2019 I agree that assert_index_equal is a part of public API, I just wasn't sure which changes needed a whatsnew note. I will add one. Thanks.

@amilbourne amilbourne changed the title Fix output from assert_frame_equal when indexes differ and check_like=True ENH; Fix output of assert_frame_equal if indexes differ and check_like=True Oct 29, 2020
@amilbourne amilbourne changed the title ENH; Fix output of assert_frame_equal if indexes differ and check_like=True ENH: Fix output of assert_frame_equal if indexes differ and check_like=True Oct 29, 2020
@amilbourne
Copy link
Contributor Author

I made the suggested changes and added a unit test for the new check_order param.

@ivanovmg ivanovmg added this to the 1.2 milestone Oct 29, 2020
Copy link
Member

@ivanovmg ivanovmg 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 to me.

@amilbourne
Copy link
Contributor Author

The build is failing due to a failing test, but it doesn't seem to be anything to do with the changes I made, so I'm assuming that is an anomaly in the build (or a flakey test or something).

The failing test is:
test_slice_irregular_datetime_index_with_nan from pandas/tests/indexing/test_partial.py

This test is also failing for other unrelated PRs, so I don't think it was me :-)

@arw2019 arw2019 added the Testing pandas testing functions or related to the test suite label Oct 29, 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.

this lgtm. just a small comment. pls rebase and ping on green.

@amilbourne
Copy link
Contributor Author

@jreback I moved the parameter as asked and rebased. The build is failing on
pandas/tests/plotting/test_datetimelike.py::TestTSPlot::test_format_timedelta_ticks_wide
But I don't think that is related to anything I changed.

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.

small comment, pls merge master and ping on green.

check_order : bool, default True
Whether to compare the order of index entries as well as their values.
If True, both indexes must contain the same elements, in the same order.
If False, both indexes must contain the same elements, but in any order.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a versionadded tag 1.2 here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's annoying, sorry.
I removed the versionadded tag when I moved the parameters :-(
I have put it back and rebased again.

@jreback jreback merged commit b867e21 into pandas-dev:master Nov 2, 2020
@jreback
Copy link
Contributor

jreback commented Nov 2, 2020

thanks @amilbourne very nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Unhelpful output from assert_frame_equal when indexes differ and check_like=True
4 participants