Skip to content

Enhance _cwt.py by introducing a configurable hop size parameter #804

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

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

phandangthoai
Copy link
Contributor

Generating a scalogram from the full output of the Continuous Wavelet Transform (CWT) entails high computational cost while providing limited performance gains in acoustic recognition models based on deep learning. Therefore, this update proposes reducing the output size during the intermediate processing stage—rather than after CWT generation—to improve computational efficiency of CWT. This pull request reflects the research findings presented in the following paper.

Phan, D. T., Huynh, T. A., Pham, V. T., Tran, C. M., Mai, V. T., & Tran, N. Q. (2025). Optimal Scalogram for Computational Complexity Reduction in Acoustic Recognition Using Deep Learning. arXiv preprint arXiv:2505.13017.

Generating a scalogram from the full output of the Continuous Wavelet Transform (CWT) entails high computational cost while providing limited performance gains in acoustic recognition models based on deep learning. Therefore, this update proposes reducing the output size during the intermediate processing stage—rather than after CWT generation—to improve computational efficiency of CWT. This pull request reflects the research findings presented in the following paper.

Phan, D. T., Huynh, T. A., Pham, V. T., Tran, C. M., Mai, V. T., & Tran, N. Q. (2025). Optimal Scalogram for Computational Complexity Reduction in Acoustic Recognition Using Deep Learning. arXiv preprint arXiv:2505.13017.
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 @phandangthoai. A few quick review comments inline. This change also needs tests, and it would be good to demonstrate what the gain of this is with a small benchmark.

@phandangthoai
Copy link
Contributor Author

Hello @rgommers,
Thanks for your quick reply with the guidelines. I am in preparation to response to your review comments accordingly.

@phandangthoai
Copy link
Contributor Author

Hello @rgommers,
Please see my responses below. I appreciate your feedback, which helps improve my pull request.

  1. I have updated the code, and all tests have passed successfully.
  2. The proposed method (optimal scalogram) is validated and benchmarked against the traditional approach (normal scalogram) using the MIMII dataset. The results indicate that a minor reduction in model performance leads to a substantial improvement in computational efficiency—achieving a speedup of nearly eight times, as illustrated in the figure below.
image image

@phandangthoai phandangthoai requested a review from rgommers July 16, 2025 13:54
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 @phandangthoai. A few inline comments. In addtion, could you please:

  1. Share a benchmark script that shows the performance improvement
  2. Add a References section in the docstring to the paper in numpydoc format

pywt/_cwt.py Outdated
@@ -24,9 +24,9 @@ def next_fast_len(n):
return 2**ceil(np.log2(n))


def cwt(data, scales, wavelet, sampling_period=1., method='conv', axis=-1):
def cwt(data, scales, wavelet, hop_size=1, sampling_period=1., method='conv', axis=-1):
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move the new keyword to the end of the signature? It's not backwards-compatible to add a new keyword in the middle. That would break calls like cwt(data, scales, wavelet, 2.5) (2.5 was the sampling period).

Also, please add it as *, hop_size=1 (the * means keyword-only).

Copy link
Member

Choose a reason for hiding this comment

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

The examples and tests then need to be updated for that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. New key word hop_size is move to the end, and defined as keyword-only.
  2. The examples and tests are revised accordingly and passed.

Move hop_size to the end
Move hop_size to the end
Add References to the research paper.
Revise the notes
@phandangthoai
Copy link
Contributor Author

Hello @rgommers ,

Thank you for your valuable feedback. I have updated the pull request in accordance with your comments.

The script for generating the optimal scalogram and the script for generating the standard scalogram have been included for reference.

A References section citing the relevant paper has been added to the function docstring, following the numpydoc format.

Please let me know if any further revisions are required.

Thank you again for your time and guidance.

Best regards,
Dang Thoai Phan

@phandangthoai
Copy link
Contributor Author

Hello @rgommers ,

I have addressed the trailing whitespace issue reported by the style check tool in my latest commit. However, it appears that the pre-commit style check did not re-trigger automatically. Could you kindly re-run it or let me know if there is anything further I need to do on my end?

Best regards,
Dang Thoai Phan

@rgommers
Copy link
Member

rgommers commented Aug 3, 2025

This accumulated a merge conflict, could you please merge with main to address that?

I've triggered the CI to run. The added tests only use hop_size=1, is that realistic enough? Seems like you'd want to use something higher as well.

Fix trailing whitespaces error
@rgommers
Copy link
Member

rgommers commented Aug 4, 2025

The merge conflict is still unresolved, so I won't retrigger CI now.

If you want to find a trivial typo or one word change somewhere that I can merge immediately in a separate PR, then CI jobs won't need manual approval from me after that.

@phandangthoai
Copy link
Contributor Author

Hello @rgommers ,

I have completed the following updates to the PR:

  • Resolved merge conflicts
  • Updated the test case to include a hop size greater than 1
  • Fixed trailing whitespace issues

Could you please review the changes and proceed with the merge into main?
Thank you for your time and support.

Best regards,
Dang Thoai Phan

Fix doctest error
@phandangthoai
Copy link
Contributor Author

Hello @rgommers,

All checks have passed, and I am thrilled to be at the stage of having my first official PR ready for merge.
I would greatly appreciate it if you could review the changes and proceed with the merge at your convenience.

Thank you for your time and support.

Best regards,
Dang Thoai Phan

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.

2 participants