Skip to content

ENH: testing: consistent names for actual and desired results#24931

Merged
ngoldbaum merged 6 commits intonumpy:mainfrom
mdhaber:gh19171
Oct 21, 2023
Merged

ENH: testing: consistent names for actual and desired results#24931
ngoldbaum merged 6 commits intonumpy:mainfrom
mdhaber:gh19171

Conversation

@mdhaber
Copy link
Contributor

@mdhaber mdhaber commented Oct 15, 2023

Closes gh-19171

gh-19171 reported that numpy.testing assertion messages sometimes referred to the actual and desired results as x and y instead of ACTUAL and DESIRED.

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_nulp and assert_array_max_ulp are 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_less does not have a notion of "actual" and "desired", but the order of the inputs matters. The arguments are named x and y, so they are referred to as such in the failure message; e.g. "Arrays are not strictly ordered x < y". (Previously, the message was less specific: "Arrays are not less-ordered".)
  • assert_allclose, assert_equal, assert_almost_equal, assert_approx_equal have arguments named actual and desired; in the failure message, the arguments are now always referred to as ACTUAL and DESIRED. (Previously, they were sometimes referred to as x and y, which was the original issue.)
  • assert_array_almost_equal and assert_array_equal have arguments named x and y. In the error message, they are now always referred to as ACTUAL and DESIRED. (Previously, they were always referrred to as x and y, 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_equal were not referred to by their names; in some sense, this problem has just been transferred to assert_array_equal and assert_array_almost_equal.

To fully resolve this, I'd like to change the names of the first two arguments of assert_array_equal and assert_array_almost_equal from x and y to actual and desired. Of course, they would still support keyword arguments x and y to preserve backward compatibility, but changing the names that show in the docstring would make the asymmetry of assert_array_equal more obvious and reduce the potential confusion of referring to x as ACTUAL and y as DESIRED in 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?)

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

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')):
Copy link
Member

Choose a reason for hiding this comment

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

Heh, this is a weird semi-public API :), but no documentation yet, so OK.

@mdhaber
Copy link
Contributor Author

mdhaber commented Oct 17, 2023

Thanks @seberg. What do you say to the backward compatible change of the x and y argument names to actual and desired (mentioned above)? I think that would more fully put this confusion to rest.

@seberg
Copy link
Member

seberg commented Oct 17, 2023

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.
(And if there are one or two exceptions it doesn't seem terrible to adapt.)

@mdhaber
Copy link
Contributor Author

mdhaber commented Oct 17, 2023

Oops, the question was not about whether the change from x/y to ACTUAL/DESIRED is backward compatible.

I was referring to the fact that there is still some confusion because the names of the first two arguments of assert_array_equal are x and y. I had proposed to change the names of these arguments to actual and desired. Of course, we would still support keyword arguments x and y to preserve backward compatibility. The rationale is that changing the names that show in the docstring would make the asymmetry of assert_array_equal more obvious and reduce the potential confusion of referring to x as ACTUAL and y as DESIRED in 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?)

@seberg
Copy link
Member

seberg commented Oct 17, 2023

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 functools.wraps will make it print the new signature, so why not... (it looks relatively heavy weight, but assertion functions are all very heavy weight)

@mdhaber
Copy link
Contributor Author

mdhaber commented Oct 17, 2023

Ok, I'll do that in a follow-up.

@ngoldbaum
Copy link
Member

@mdhaber this looks good to me! I'll merge it after you fix the conflicts - there was another PR that touched the internals of assert_array_less and updated some of these docstrings.

I have a slight preference to make the functions positional-only and print deprecation warnings when someone uses x or y keywords, but don't care too much.

@mdhaber
Copy link
Contributor Author

mdhaber commented Oct 21, 2023

Thanks @ngoldbaum. Where should the decorator for the rename/deprecation go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

np.testing error messages lose "Actual" and "Desired" metadata when comparing arrays

4 participants