FIX: scaling factor for window with negative value#25122
FIX: scaling factor for window with negative value#25122oscargus merged 5 commits intomatplotlib:mainfrom
Conversation
Simply drop the `np.abs()` on window to fix the wrong scaling factor for window with negative value. For more detail refer to matplotlib#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).
|
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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Add new test function for scale factor of flattop window. Also remove the unnecessary `np.abs()` on window in other functions.
|
I have add the new function |
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
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.
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
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...
|
Thanks @gapplef and congratulations on your first merged PR! Hope to see you around! |
4 participants
PR Summary
Simply drop the
np.abs()on window to fix the wrong scaling factor for window with negative value. For more detail refer to #24821Caution: 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.
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
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst