NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

FIX: scaling factor for window with negative value#25122

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

oscargus merged 5 commits intomatplotlib:mainfromgapplef:patch-1

Feb 7, 2023

Merged

FIX: scaling factor for window with negative value#25122

oscargus merged 5 commits intomatplotlib:mainfromgapplef:patch-1

Feb 7, 2023

Conversation

Copy link

Contributor

gapplef commentedFeb 1, 2023•
edited by oscargus
Loading

PR Summary

Simply drop thenp.abs() on window to fix the wrong scaling factor for window with negative value. For more detail refer to#24821

Caution: With this fix, the behavior would change for window with complex value.
Withnp.abs() on window, it seems can handle complex value, but I don't think it's the right way to do it. As it can't fall back to real value case for complex value with zero imaginary part and negative real part (for example -1 + 0j).
Also, I didn't understand the need for complex window, so here I simply ignore the complex case. And this is consistent with the implementation ofscipy.

Closes#24821

Related to#22828 (not sure it can be closed, that seems to try to deal with the complex case as well, but is there one?).

PR Checklist

Documentation and Tests

Has pytest style unit tests (andpytest passes)
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
New plotting related features are documented with examples.

Release Notes

New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

FIX: scaling factor for window with negative value

cebd3d7

Simply drop the `np.abs()` on window to fix the wrong scaling factor for window with negative value.For more detail refer tomatplotlib#24821**Caution**: With this fix, the behavior would change for window with complex value.With `np.abs()` on window, it seems can handle complex value, but I don't think it's the right way to do it. As it can't fall back to real value case for complex value with zero imaginary part and negative real part (for example -1 + 0j).Also, I didn't understand the need for complex window, so here I simply ignore the complex case. And this is consistent with the implementation of [scipy](https://github.com/scipy/scipy/blob/d9f75db82fdffef06187c9d8d2f0f5b36c7a791b/scipy/signal/_spectral_py.py#L1854-L1859).

Copy link

Member

jklymak commentedFeb 1, 2023

I think these are correct. Can you add some tests, particularly if you can somehow create a flattop w/o using scipy (we don't use it for our tests, unfortunately). For tests, I'd calculate the spectrum of random numbers and just show that its average equals an expected value, perhaps with a bit of floating point slop (np.testing.allclose)?

I don't think this needs a what's new or anything - it only applies to windows with negative co-efficients, which are relatively rare.

jklymak added the topic: spectral label

Feb 1, 2023

jklymak added this to thev3.7.1 milestone

Feb 1, 2023

github-actionsbot reviewed

Feb 1, 2023

View reviewed changes

Copy link

github-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

gapplef mentioned this pull request

Feb 1, 2023

[Bug]: Windows correction is not correct inmlab._spectral_helper#24821

Closed

Test function for scale factor of flattop window

b2c8f88

Add new test function for scale factor of flattop window.Also remove the unnecessary `np.abs()` on window in other functions.

Copy link

ContributorAuthor

gapplef commentedFeb 1, 2023

I have add the new functiontest_psd_window_flattop, with reference totest_psd_window_hanning andtest_psd_windowarray_scale_by_freq.

fix bug intest_psd_window_flattop

76e53c1

jklymak reviewed

Feb 1, 2023

View reviewed changes

Copy link

Member

jklymak left a comment

Choose a reason for hiding this comment

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

Looks close to me. Not sure about some of the testing details so please check. Note we expect all the code in tests to be run, so please remove the dead block.

lib/matplotlib/tests/test_mlab.pyShow resolvedHide resolved

lib/matplotlib/tests/test_mlab.py OutdatedShow resolvedHide resolved

update code formatting fortest_psd_window_flattop

456c4cf

melissawm mentioned this pull request

Feb 2, 2023

Feature request: PR comments with build preview website linknapari/docs#98

Closed

jklymak approved these changes

Feb 5, 2023

View reviewed changes

Copy link

Member

jklymak left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and discussion that lead to it! The lint issue needs to be dealt with, and we need a second review, but I believe this is correct and should go in...