Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional documentation for cwt (issue #676) #678

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

drs251
Copy link
Contributor

@drs251 drs251 commented Aug 20, 2023

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:

  • Added end-to-end example of generating a chirp signal, performing a CWT, plotting the scaleogram with matplotlib, and compare to a Fourier Transform.
  • A bit of introductory text, and a link to Torrence & Compo's Practical Guide to Wavelet Analysis.
  • A plot of all available wavelets for CWT, and a comparison of the outcome using wavelets with different center frequencies and bandwidths. (for some reason, generating the corresponding images with a Python script slowed down Sphinx massively, so I ended up using pre-made images, as I could not figure out a better solution).
  • While I was at it, I could not resist to increase the depth of the table of contents in index.rst to 2 - this makes it much easier to find individual pages.

Looking forward to constructive criticism.

@drs251
Copy link
Contributor Author

drs251 commented Aug 21, 2023

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
Copy link
Member

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

@rgommers
Copy link
Member

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
Copy link
Contributor Author

drs251 commented Aug 22, 2023

@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
Copy link
Contributor Author

drs251 commented Aug 27, 2023

@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
Copy link
Member

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
Copy link
Contributor Author

drs251 commented Sep 2, 2023

@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?

@rgommers rgommers added this to the v1.5.0 milestone Sep 15, 2023
@drs251
Copy link
Contributor Author

drs251 commented Oct 12, 2023

@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 rgommers modified the milestones: v1.5.0, v1.6.0 Feb 22, 2024
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @drs251. Sorry about the long delay - neither me nor another maintainer had any time for PyWavelets around the time you opened this PR.

I now resolved the merge conflicts, did a copy-edit, and made some minor changes to plot_wavelets.py to avoid using deprecated forms of string wavelet names.

The added content is quite nice, thank you.

A plot of all available wavelets for CWT, and a comparison of the outcome using wavelets with different center frequencies and bandwidths. (for some reason, generating the corresponding images with a Python script slowed down Sphinx massively, so I ended up using pre-made images, as I could not figure out a better solution).

It's not super fast, but it doesn't bother me much right now - total doc build time seems reasonable.

While I was at it, I could not resist to increase the depth of the table of contents in index.rst to 2 - this makes it much easier to find individual pages.

In the meantime we just changed to a new doc theme and moved the toctree to the top of the page, plus have a better navbar. I hope that helps with the discoverability. If not, let's deal with this separately. :maxdepth: 2 wass too much.

@rgommers rgommers merged commit c2aa039 into PyWavelets:master Mar 8, 2024
15 checks passed
@rgommers
Copy link
Member

rgommers commented Mar 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants