ENH: assert_array_less should report max violations instead of max differences #24369
Conversation
|
Looks like some docstrings need to be updated, the test failures are real. |
|
ok updated, is the travis ci pull request build failure an issue on my end too? |
|
Nope, that one's unrelated. The Travis PPC64LE runners have been flaky for quite a while |
There was a problem hiding this comment.
I think this is a clear improvement, thank you for working on this, it will make the debugging experience for the entire PyData ecosystem nicer when a test using this assert fails.
The implementation looks correct and the new tests make sense. I think you can substantially slim down the new tests you've added by checking for a matching message directly in pytest.raises, see comments below.
numpy/testing/tests/test_utils.py
Outdated
| self._assert_func(x, y) | ||
| msg = str(exc_info.value) | ||
| assert_('Mismatched elements: 1 / 1 (100%)\n' | ||
| in msg) |
There was a problem hiding this comment.
nit, here and below: you can pass a match=some_regex to pytest.raises to check for a matching message.
There was a problem hiding this comment.
Thank you for the review, i have updated the error message checks to use match
numpy/testing/tests/test_utils.py
Outdated
| # assert_('Mismatched elements: 2 / 2 (100%)\n' | ||
| # 'Max absolute difference among violations: 2.\n' | ||
| # 'Max relative difference among violations: inf' | ||
| # in msg) |
There was a problem hiding this comment.
I have uncommented this out, just to confirm the mismatched elements is 1 / 2 and max relative difference among violations should be inf right? (as per the results in the original numpy on the main branch)
|
Hi, any updates on this pr? thank you |
ngoldbaum
added
the
triage review
|
I added the triage review label so hopefully this will get a look at the next triage meeting unless someone else merges in the meantime. This is a lot of code so I want another set of eyes before merging. |
|
ok, thank you |
|
It looks like there's a conflict that needs to be fixed |
Went ahead and fixed it, I'll merge if this is still passing all the tests. |
|
Thanks @ellaella12! |
Fixes #23894