PyWavelets / pywt

PyWavelets - Wavelet Transforms in Python
http://pywavelets.readthedocs.org
MIT License
1.97k stars 460 forks source link

Additional documentation for cwt (issue #676) #678

Closed drs251 closed 3 months ago

drs251 commented 10 months ago

This PR is in response to issue #676. Basically, I wrote the documentation I wish I had when I started messing with the CWT πŸ˜„. Here's a summary of the proposed changes:

Looking forward to constructive criticism.

drs251 commented 10 months ago

Wow, I was not expecting test failures, since I only touched the documentation. Seems like there is a warning since version 3.9 about some out-of-date .npy or .npz files, which is causing the failures. How should we proceed from here?

rgommers commented 10 months ago

Thanks for this PR @drs251. Let me resolve the failures in a separate PR, and then rebase this one on top.

rgommers commented 10 months ago

This is green now πŸ˜…. @drs251 did you commit the .png files on purpose? The doc/source/pyplots/ directory contains only scripts, and those should get run at html doc build time in order to recreate the plots and include them in the docs. That way we keep the repository lean, and we can verify that the code actually runs to produce those plots.

drs251 commented 10 months ago

@rgommers Thanks for fixing the test failures so quickly. Regarding the .png files: I wrote doc/source/pyplots/cwt_wavelet_frequency_bandwidth_demo.py initially to generate the two plots in question (included in the PR). But I noticed, that this script causes sphinx to run several minutes longer. I could not figure out the cause, so I decided to include the .png files instead. If we don't care about the sphinx runtime, or if you're able to suggest a solution, I have no problems removing the .pngs and using the script instead to generate the plots again. (As a small downside, some of the code from the first example would be repeated, but this is not a huge problem IMO).

drs251 commented 10 months ago

@rgommers I got rid of the .png files and replaced them with doc/source/pyplots/cwt_wavelet_frequency_bandwidth_demo.py, as you suggested. I decreased the resolution to improve the sphinx runtime: the runtime was 22 seconds on my machine before the new plots, and now it is 1:08 min. I confirmed that this increase is caused only by doc/source/pyplots/cwt_wavelet_frequency_bandwidth_demo.py, even though the script only takes 3 seconds when run on its own. I put a comment in about this in cwt.rst. Might be a bug in sphinx, perhaps it's worth opening an issue.

rgommers commented 10 months ago

Thanks! Sphinx had a lot of regressions in 7.2.X recently, so that may be it. As long as the lower resolution still looks good on screen, that should be fine though. No need to dig into the potential Sphinx issue I'd think.

drs251 commented 10 months ago

@rgommers I'm actually happy with the way the plot looks, the resolution is good enough to convey the point: cwt_wavelet_frequency_bandwidth_demo_01_00

Is there anything left to do before the PR can be merged?

drs251 commented 8 months ago

@rgommers Just wondering if there anything left to do before the PR can be merged? From my point of view, itβ€˜s ready to go πŸ˜„

rgommers commented 3 months ago

Merged in preparation for the next release. Thanks again @drs251!