Skip to content

ENH: Modified metadata for build_err_msg method#20161

Closed
sayantikabanik wants to merge 2 commits intonumpy:mainfrom
sayantikabanik:metadata_update
Closed

ENH: Modified metadata for build_err_msg method#20161
sayantikabanik wants to merge 2 commits intonumpy:mainfrom
sayantikabanik:metadata_update

Conversation

@sayantikabanik
Copy link
Contributor

@sayantikabanik sayantikabanik commented Oct 22, 2021

Info regarding the PR

  • PR opened for the issue 19171
  • Replaced the x & y params with ACTUAL & DESIRED for clear understanding during debugging
  • modified tests under tests_utils.py to incorporate the message string change

@sayantikabanik sayantikabanik changed the title ENG: Modified metadata for build_err_msg method ENH: Modified metadata for build_err_msg method Oct 22, 2021
@czgdp1807
Copy link
Member

czgdp1807 commented Oct 22, 2021

The following line (and similar other examples in the whole project where traceback is shown for test failures) needs change (x -> 'ACTUAL', y -> 'DESIRED')

x: array([1. , 2.33333, nan])
y: array([1. , 2.33339, nan])

@czgdp1807
Copy link
Member

Seems like numpy/testing/_private/utils.py", line 1004, in assert_array_almost_equal also requires the above change.

@czgdp1807
Copy link
Member

Searching for BLANKLINE in the entire project in VS Code, I found 7 examples which might require x -> ACTUAL and y -> DESIRED change. I haven't updated my master yet though.

@sayantikabanik
Copy link
Contributor Author

I see them under, not sure if these should be updated

Screenshot 2021-10-22 at 8 30 07 PM

@czgdp1807
Copy link
Member

Seems like you have already made the replacement where the CI is showing mismatch error. It's happening only on macOS I think. I don't think there is anything else to be done.

@czgdp1807
Copy link
Member

In the following line. I think names aren't updated to ACUTAL and DESIRED. May be, that's why doc-tests are failing.

names=('x', 'y'), precision=precision)

@czgdp1807
Copy link
Member

You can apply the following diff (use git apply file_containing_the_following.diff)

diff --git a/numpy/testing/_private/utils.py b/numpy/testing/_private/utils.py
index af9e31e77..162debfe5 100644
--- a/numpy/testing/_private/utils.py
+++ b/numpy/testing/_private/utils.py
@@ -741,7 +741,7 @@ def func_assert_same_pos(x, y, func=isnan, hasval='nan'):
             msg = build_err_msg([x, y],
                                 err_msg + '\nx and y %s location mismatch:'
                                 % (hasval), verbose=verbose, header=header,
-                                names=('x', 'y'), precision=precision)
+                                names=('ACTUAL', 'DESIRED'), precision=precision)
             raise AssertionError(msg)
         # If there is a scalar, then here we know the array has the same
         # flag as it everywhere, so we should return the scalar flag.
@@ -759,7 +759,7 @@ def func_assert_same_pos(x, y, func=isnan, hasval='nan'):
                                 err_msg
                                 + f'\n(shapes {x.shape}, {y.shape} mismatch)',
                                 verbose=verbose, header=header,
-                                names=('x', 'y'), precision=precision)
+                                names=('ACTUAL', 'DESIRED'), precision=precision)
             raise AssertionError(msg)
 
         flagged = bool_(False)

@czgdp1807
Copy link
Member

Other than that, I get the following error on your branch. Not sure if it's related. Just commenting anyways.

numpy.core.ndarray
------------------

File "build/testenv/lib/python3.9/site-packages/numpy/__init__.py", line ?, in ndarray.__class_getitem__
Failed example:
    np.ndarray[Any, np.dtype[Any]]
Expected:
    numpy.ndarray[typing.Any, numpy.dtype[Any]]
Got:
    numpy.ndarray[typing.Any, numpy.dtype[typing.Any]]


ERROR:  failed checking numpy.core

Copy link
Member

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

This looks good to me. @BvB93 @almostsquare Any thoughts on this?

@BvB93
Copy link
Member

BvB93 commented Oct 25, 2021

@czgdp1807 on which python version are you receiving this error?
This far I've been unable to reproduce this error on Python 3.9.1 and 3.9.7 with the latest upstream/main branch.

@czgdp1807
Copy link
Member

@czgdp1807 on which python version are you receiving this error?
This far I've been unable to reproduce this error on Python 3.9.1 and 3.9.7 with the latest upstream/main branch.

I get it with 3.9.6.

@BvB93
Copy link
Member

BvB93 commented Oct 25, 2021

Oh, wait a minute, I see now what's going on here.
I'll open a PR in a bit.

In any case, don't the __class_getitem__ refguide failure get into the way of this PR.

@sayantikabanik
Copy link
Contributor Author

@czgdp1807 @BvB93 are there any more changes required for this PR to be complete?

@almostsquare
Copy link

This looks good to me. @BvB93 @almostsquare Any thoughts on this?

Thanks for the changes, @sayantikabanik! Looks good!

@czgdp1807
Copy link
Member

LGTM.

@mattip
Copy link
Member

mattip commented Nov 1, 2021

Since this is user-facing, do we want to add a one-line release note about the changed behaviour?

@sayantikabanik
Copy link
Contributor Author

Since this is user-facing, do we want to add a one-line release note about the changed behaviour?

I second that.

@charris
Copy link
Member

charris commented Nov 12, 2021

@sayantikabanik You can make the release note in doc/release/upcoming_changes. Look at the other notes to see how it is done.

@sayantikabanik sayantikabanik force-pushed the metadata_update branch 3 times, most recently from 9a58de1 to 59df97c Compare November 12, 2021 07:28
@czgdp1807
Copy link
Member

This needs a clean up of git history. We will meet later today and help you clean up the stuff here.

@sayantikabanik
Copy link
Contributor Author

This needs a clean up of git history. We will meet later today and help you clean up the stuff here.

Thanks :D

@sayantikabanik
Copy link
Contributor Author

Could the CI be restarted from the backend? It's stuck I think.

@czgdp1807
Copy link
Member

It's Travis again. @rgommers It's get stuck very often. Can be ignored I think.

@mdhaber
Copy link
Contributor

mdhaber commented Oct 7, 2023

It looks like this was close and valuable for consistency.
@sayantikabanik would you like to merge main here? That should get CI going again.
I could also open a new PR to do this - preserving your commits - if you'd prefer. (Update: Done.)

@mdhaber
Copy link
Contributor

mdhaber commented Oct 21, 2023

@ngoldbaum looks like this can be closed now

@ngoldbaum ngoldbaum closed this Oct 21, 2023
@ngoldbaum
Copy link
Member

Finished in #24931

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

8 participants