8000 Missed DeprecationWarning, PendingDeprecationWarning in NumPy tests · Issue #10185 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

Missed DeprecationWarning, PendingDeprecationWarning in NumPy tests #10185

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.

Sign up for GitHub

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

Jump to bottom
Closed
charris opened this issue Dec 9, 2017 · 25 comments · Fixed by #10193
Closed

Missed DeprecationWarning, PendingDeprecationWarning in NumPy tests #10185

charris opened this issue Dec 9, 2017 · 25 comments · Fixed by #10193
Labels
00 - Bug

Comments

@charris
Copy link
Member
charris commented Dec 9, 2017

These show up release tests, and should raise errors in development, but they don't. We must be messing with the warnings queue somewhere.

test_nat_items (numpy.testing.tests.test_utils.TestEqual) ... /home/charris/Workspace/numpy.git/build/testenv/lib64/python2.7/site-packages/numpy/testing/nose_tools/utils.py:407: DeprecationWarning: elementwise == comparison failed; this will raise an error in the future.
  if not (desired == actual):

and

numpy.core.tests.test_multiarray.TestFormat.test_1d_format ... /home/charris/Workspace/numpy.git/build/testenv/lib64/python2.7/site-packages/numpy/core/tests/test_multiarray.py:7092: PendingDeprecationWarning: object.__format__ with a non-empty format string is deprecated
  return f(*args, **kwargs), None
@njsmith
Copy link
Member
njsmith commented Dec 9, 2017 via email

@seberg
Copy link
Member
seberg commented Dec 9, 2017 •
edited
Loading

But through some error handling all is good when no warning is given (didn't quite track it down yet)? The second one, I don't see right now (probably it depends on py version or so). EDIT: Sorry, no error is given.

@seberg
Copy link
Member
seberg commented Dec 9, 2017

Ah, the _GenericTest in test_utils catches AssertionErrors (which the DeprecationWarning when raised would be converted into probably. The strange thing is that the DeprecationWarning is converted to an AssertionError maybe, hmmm

@charris
Copy link
Member Author
charris commented Dec 9, 2017

The first one should also be caught in the assert_equal function, as it involves NaT == NaT, which we should not actually reach.

@charris
Copy link
Member Author
charris commented Dec 9, 2017

The second one is, I think, a Python PendingDeprecationWarning , yep, comes from

        # Could switch on python version here, but all we care about is
        # that the behaviour hasn't changed
        assert_equal(
            ret_and_exc(object.__format__, a, '30'),
            ret_and_exc('{:30}'.format, a)
        )

We should probably just remove that, as testing against object.__format__ is going to be deprecated.

@seberg
Copy link
Member
seberg commented Dec 9, 2017 •
edited by eric-wieser
Loading

Well, I guess what causes this is the tests that compare [NaT] to NaT. @eric-wieser, Eric, you seemed to have put the replacement of FutureWarning and DeprecationWarning there at the end when fixing up my NaT mess (#10096), do you remember off the top of your head why that was necessary?

@seberg
Copy link
Member
seberg commented Dec 9, 2017

Probably can just remove that test, true, though I find it a bit odd that PendingDeprecationWarning did not show up before? The tests should error on pretty much all warnings? Or maybe nobody was testing against a new enough python since then?

@eric-wieser
Copy link
Member

as testing against object.format is going to be deprecated.

Is that a python change?

@eric-wieser
Copy link
Member

Well, I guess what causes this is the tests that compare [NaT] to NaT.

I suppose this is in assert_array_equal, since one of the elements is an array?

do you remember off the top of your head why that was necessary?

It was to allow assert_equal to be used on types with no comparison defined without warning

@charris
Copy link
Member Author
charris commented Dec 9, 2017

@seberg In order to see them, you need to have ISRELEASED=True in setup.py .

@eric-wieser
Copy link
Member

I guess I decided that I didn't need to suppress the warnings, only when they were exceptions - I don't think I had a strong rationale behind that

@charris
Copy link
Member Author
charris commented Dec 9, 2017

@eric-wieser , it is actually in assert_equal and is because the case of two NaT with different types is not caught as an error. It's an easy fix.

@eric-wieser
Copy link
Member

I think the warning suppression is still needed there to deal with assert_equal(void, datetime) or other types where no == loop can be found

@eric-wieser
Copy link
Member

The __format__ thing is #9883 - also my doing

@charris
Copy link
Member Author
charris commented Dec 9, 2017 •
edited
Loading

Is that a python change?

Yes, the message isn't in NumPy.

EDIT: Isn't in old python 2.7 either, looks like it must be a recent backport from python 3.

@seberg
Copy link
Member
seberg commented Dec 9, 2017

@charris, python3 runtests.py --raise-warnings=release should work as well. But in develop mode it should show up as an error, why doesn't it? (Maybe it does not matter, I might try to understand it myself later)

@eric-wieser why do we need to convert it to AssertionError? The other tests then catches AssertionErrors.
Unfortunately we cannot suppress warnings in those testing functions, because warning suppression is not thread safe, so if there is some change necessary there, it might end up being quite a dance.

@eric-wieser
Copy link
Member

If we don't convert it to AssertionError, then we never see the message that tells us what the values are, which makes assertEqual kind of useless.

@charris
Copy link
Member Author
charris commented Dec 9, 2017

@seberg It doesn't show up because the test is constructed to succeed if the assertion fails. Kind of weird, and not designed to work with deprecations.

@charris
Copy link
Member Author
charris commented Dec 10, 2017

@eric-wieser I think we should simply re-raise the error if assert_equal fails due to PendingDeprecationWarning or DeprecationWarning in development mode. The current problem arises because the test that fails is using AssertionError to check non-equality. In release mode both of those are merely warnings instead of failures. We should be able to append the relevant message someplace in there.

@eric-wieser
Copy link
Member
eric-wieser commented Dec 10, 2017 •
edited
Loading

because the test that fails is using AssertionError to check non-equality.

Can't we just change this?

@charris
Copy link
Member Author
charris commented Dec 10, 2017

Maybe, I'm going to take a look ...

@eric-wieser
Copy link
Member

Hmm, actually this seems to be a pretty common thing to do in the meta-tests that test the assertion helpers.
I think the right thing to do is suppress the warning

@charris
Copy link
Member Author
charris commented Dec 10, 2017

I think it already does the correct thing in release mode, the equality check goes through and a warning is issued. It is development mode where the warnings are converted to AssertionError that is the problem.

@charris
Copy link < AB21 div aria-live="polite" aria-atomic="true" class="sr-only" data-clipboard-copy-feedback>
Member Author
charris commented Dec 10, 2017

Not that the NaT == NaT is fixed by handling it in the NaT comparison section, the problem is that we didn't catch that error during development.

@charris charris mentioned this issue Dec 10, 2017
Merged
@charris
Copy link
Member Author
charris commented Dec 10, 2017

@eric-wieser I put up some fixes at #10193, if you want to do more with assert_equal you should comment there.

@charris charris mentioned this issue Dec 11, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
00 - Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BUG: Fix bugs found by testing in release mode. charris/numpy
4 participants
@seberg @charris @eric-wieser @njsmith
0