Allow setting frequency units for psd plots - #32211
Conversation
In psd, allow user to specify the units for the sampling frequency of the analyzed array. psd previously assumed units to be Hz and displayed this on the y-axis label. Users can now specify other units. This change has no effect on the psd calculation.
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat 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. |
scottshambaugh
left a comment
There was a problem hiding this comment.
One comment to avoid an API break, but I think this looks pretty good and is a nice quality-of-life improvement.
|
TBH I didn't know that we had this function, looks like it's in here mainly for MATLAB compatibility. Normally we don't deal with units so I'd be tempted to deprecate it, but I think it's fine for a compatibility function. There are some references to Hz in the comments of |
We support physical units through the units module, so I'm curious why that wouldn't work/can't be patched in here. Using unit data should put those units in the label b/c the unit's class handles labeling. |
|
I think we should have deprecated these functions years ago. Folks should use scipy.signal and then plot as they need for their field. Some of the calculations are incorrect and no one has the time or means to maintain this properly and fix bugs. See #22920 |
If the problem is that this needs replacement , could we do something like requiring scipy only if you're using this part of the API? It would allow us to shim in the scipy piece w/o making it a hard requirement. |
|
I don't think this needs replacement. It should just be removed. This was a Matlab leftover from before scipy existed. If someone wanted to write a downstream wrapper around Matplotlib and scipy I think that would be valuable, but I would not recommend these routines for general use. |
|
I seem to have unintentionally begun (or restarted) a larger discussion. As suggested by @scottshambaugh, I edited the mlab comments so that Hz frequency units are used as an example, but not assumed. I have no plan to develop these functions further, apart from any minor edits to complete this PR. While I think this small change is useful now, I respect your decision about whether to merge this PR, given the long-term vision of replacing or deprecating/removing these functions. Thanks for your consideration. |
PR summary
This PR allows users to specify the units for the sampling frequency in power spectral density (psd) plots. psd previously assumed units to be Hz and displayed Hz on the y-axis label. Users can now specify other units with the
Funitskeyword. If the keyword is omitted, Hz is assumed by default, so the change is backward compatible. This change has no effect on the psd calculation, only the resulting plot.AI Disclosure
No AI was used
PR quality check