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

Improve error message for array_equal asserts. #4973

Conversation

csadorf
Copy link
Contributor

@csadorf csadorf commented Nov 4, 2022

No description provided.

@csadorf csadorf added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 4, 2022
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Nov 4, 2022
@csadorf csadorf marked this pull request as ready for review November 4, 2022 19:21
@csadorf csadorf requested a review from a team as a code owner November 4, 2022 19:21
@wphicks
Copy link
Contributor

wphicks commented Nov 7, 2022

I like this change generally, but we should look at the CI failures not just to fix them but to make sure we like the error formatting we're seeing.

@csadorf
Copy link
Contributor Author

csadorf commented Nov 7, 2022

I like this change generally, but we should look at the CI failures not just to fix them but to make sure we like the error formatting we're seeing.

Good point. The problem is that right now this will only print nice errors if the repr is explicitly printed, i.e., to not show \n, but print an actual line break. I think it might be possible to generally get a nicer output by implementing a custom pytest_assertrepr_compare for this class.

@wphicks
Copy link
Contributor

wphicks commented Nov 7, 2022

I think it might be possible to generally get a nicer output by implementing a custom pytest_assertrepr_compare for this class.

Totally fine to leave that for a followup. If you're going to do so, though, just be sure to create an associated issue.

@csadorf
Copy link
Contributor Author

csadorf commented Nov 7, 2022

I think it might be possible to generally get a nicer output by implementing a custom pytest_assertrepr_compare for this class.

Totally fine to leave that for a followup. If you're going to do so, though, just be sure to create an associated issue.

I'll try to address that immediately.

@csadorf
Copy link
Contributor Author

csadorf commented Nov 7, 2022

Not quite happy with the latest change yet, as it encourages array_equal == True patterns.

@csadorf csadorf marked this pull request as draft November 7, 2022 19:08
 - Always concatenate split lines for __repr__ strings.
 - Use brackets instead of hyphens and use whitespace-delimitation to
   not imply that the representation is eval'able.
@csadorf csadorf marked this pull request as ready for review November 8, 2022 15:45
@csadorf
Copy link
Contributor Author

csadorf commented Nov 8, 2022

@wphicks This would be ready for another review. I have adjusted the error message slightly and also added a assert_array_equal function in analogy to np.testing.assert_equal which would probably be preferred over assert array_equal. There is unfortunately currently no way to hack pytest to show a custom error message for simple asserts (not a comparison), see pytest-dev/pytest#5535 .

Edit: Clarifying assert_array_eqal would be preferred over assert array_equal

@csadorf csadorf requested a review from wphicks November 8, 2022 15:49
@wphicks
Copy link
Contributor

wphicks commented Nov 9, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0beb45f into rapidsai:branch-22.12 Nov 9, 2022
@csadorf csadorf deleted the fea-better-error-message-for-array-equal-asserts branch November 9, 2022 16:13
ahendriksen pushed a commit to ahendriksen/cuml that referenced this pull request Nov 9, 2022
ahendriksen pushed a commit to ahendriksen/cuml that referenced this pull request Nov 10, 2022
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants