-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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.
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
Catch specgram warnings during tests #7985
Conversation
lib/matplotlib/tests/test_mlab.py
Outdated
'07/09/83 5:17:34 PM\n' + | ||
'06/20/2054 2:31:45 PM\n' + | ||
'10/31/00 11:50:23 AM\n') | ||
'03/05/76 12:00:01 AM\n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may as well remove the "+" to rely on implicit string concatenation.
lib/matplotlib/tests/test_mlab.py
Outdated
@@ -2103,6 +2104,14 @@ def test_specgram_phase(self): | |||
assert spec.shape[0] == freqs.shape[0] | |||
assert spec.shape[1] == self.t_specgram.shape[0] | |||
|
|||
def test_specgram_warn_only1seg(self): | |||
"""Warning should be raised if len(x) <= len(NFFT). """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<= NFFT I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of Course. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, apart from the extra warning statement needed
lib/matplotlib/tests/test_mlab.py
Outdated
def test_specgram_warn_only1seg(self): | ||
"""Warning should be raised if len(x) <= NFFT. """ | ||
with warnings.catch_warnings(record=True) as w: | ||
mlab.specgram(x=self.y, NFFT=len(self.y), Fs=self.Fs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just before this line will need a line with
warnings.simplefilter('always')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Thanks.
This all looks good now. To reviewer+1: I can't get the old warnings to appear using pytest on master anymore, does anyone know why this is? |
I think pytest does a better job of capturing output than nose did; you can try passing |
Yep, I'm picking them up with the pytest-warnings plugin now. |
The nosig_complex_twosided_nopad_to and nosig_complex_defaultsided_nopad_to tests appear to have been mistakes, because they repeat the parameters used in nosig_complex_twosided_noNFFT_no_pad_to and nosig_complex_defaultsided_noNFFT_no_pad_to, so change `None` to `-1`. The nosig_complex_defaultsided_trim test was somehow not causing warnings and thus not "fixed" by matplotlib#7985, but change it `len_x=1024` and `NFFT_density=512` to be consistent with the other tests. The nosig_complex_defaultsided_oddlen test has always been different from its sibling tests and I suspect it was an accident, so change `NFFT_density` to 33. Fixes matplotlib#8393.
The nosig_complex_twosided_nopad_to and nosig_complex_defaultsided_nopad_to tests appear to have been mistakes, because they repeat the parameters used in nosig_complex_twosided_noNFFT_no_pad_to and nosig_complex_defaultsided_noNFFT_no_pad_to, so change `None` to `-1`. The nosig_complex_defaultsided_trim test was somehow not causing warnings and thus not "fixed" by matplotlib#7985, but change it `len_x=1024` and `NFFT_density=512` to be consistent with the other tests. The nosig_complex_defaultsided_oddlen test has always been different from its sibling tests and I suspect it was an accident, so change `NFFT_density` to 33. Fixes matplotlib#8393.
This catches the
specgram()
warning introduced in #7845 intest_mlab
. Also an additional test was added to check if the warning is raised. Closes #7967.