ENH: testing: consistent names for actual and desired results#24931
ENH: testing: consistent names for actual and desired results#24931ngoldbaum merged 6 commits intonumpy:mainfrom
Conversation
[skip actions] [skip cirrus]
seberg
left a comment
There was a problem hiding this comment.
LGTM, I am a bit sad that the helper is public API :). Gonna wait a bit to see if someone else wants to have a look.
| def assert_array_compare(comparison, x, y, err_msg='', verbose=True, header='', | ||
| precision=6, equal_nan=True, equal_inf=True, | ||
| *, strict=False): | ||
| *, strict=False, names=('ACTUAL', 'DESIRED')): |
There was a problem hiding this comment.
Heh, this is a weird semi-public API :), but no documentation yet, so OK.
|
Thanks @seberg. What do you say to the backward compatible change of the |
|
I don't care much, changing prints in tests just doesn't register to me like a worrying change, I don't see why users would test a test failure. |
|
Oops, the question was not about whether the change from I was referring to the fact that there is still some confusion because the names of the first two arguments of The decorator we use in SciPy for this purpose, |
|
Oh, right. I agree that should go to a deprecation (either rename or only allow passing positional, which honestly seems fine also). Didn't try to make up my mind if I like the decorator much but I at least the |
|
Ok, I'll do that in a follow-up. |
[skip actions] [skip cirrus]
|
@mdhaber this looks good to me! I'll merge it after you fix the conflicts - there was another PR that touched the internals of I have a slight preference to make the functions positional-only and print deprecation warnings when someone uses |
|
Thanks @ngoldbaum. Where should the decorator for the rename/deprecation go? |
Closes gh-19171
gh-19171 reported that
numpy.testingassertion messages sometimes referred to the actual and desired results asxandyinstead ofACTUALandDESIRED.gh-20161 appeared to fix this issue and seemed close to being merged, but it became inactive.
This PR squashes the commits from the original PR and resolves merge conflicts. I also made additional changes required to improve consistency.
Here is the state of things:
assert_array_almost_equal_nulpandassert_array_max_ulpare symmetric with respect to the order of the inputs. The failure messages do not refer to the arguments by name; e.g. "Arrays are not equal to 1 ULP". (Previously, one of these referred to the arguments by name; the other did not.)assert_array_lessdoes not have a notion of "actual" and "desired", but the order of the inputs matters. The arguments are namedxandy, so they are referred to as such in the failure message; e.g. "Arrays are not strictly orderedx < y". (Previously, the message was less specific: "Arrays are not less-ordered".)assert_allclose,assert_equal,assert_almost_equal,assert_approx_equalhave arguments namedactualanddesired; in the failure message, the arguments are now always referred to asACTUALandDESIRED. (Previously, they were sometimes referred to asxandy, which was the original issue.)assert_array_almost_equalandassert_array_equalhave arguments namedxandy. In the error message, they are now always referred to asACTUALandDESIRED. (Previously, they were always referrred to asxandy, but this is not as bad as the original issue because the docstrings refer to these arguments as the "actual" and "desired" objects. )Regarding the last point - the original issue was that the arguments of
assert_equalwere not referred to by their names; in some sense, this problem has just been transferred toassert_array_equalandassert_array_almost_equal.To fully resolve this, I'd like to change the names of the first two arguments of
assert_array_equalandassert_array_almost_equalfromxandytoactualanddesired. Of course, they would still support keyword argumentsxandyto preserve backward compatibility, but changing the names that show in the docstring would make the asymmetry ofassert_array_equalmore obvious and reduce the potential confusion of referring toxasACTUALandyasDESIREDin the failure message.The decorator we use in SciPy for this purpose,
_rename_parameter, would make this easy. Shall I do this in a follow-up PR? (Does NumPy have its own decorator for this purpose, and if not, should I vendor SciPy's decorator?)