Repository navigation
FIX: scaling factor for window with negative value - #25122
oscargus merged 5 commits into
Conversation
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
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 test_psd_window_flattop, with reference to test_psd_window_hanning and test_psd_windowarray_scale_by_freq. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
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...
Sorry, something went wrong.
|
Thanks @gapplef and congratulations on your first merged PR! Hope to see you around! |
Sorry, something went wrong.
Sorry, something went wrong.
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 #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.
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
Release Notes