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

window type is changed to hamming in discontinuity detection. #1339

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AhmetOguzOzturk
Copy link

In DiscontinuityDetector.cpp file, triangular window type is changed to Hamming, after the study of my master thesis.

@palonso
Copy link
Contributor

palonso commented Jun 21, 2023

Thank you for your contribution @AhmetOguzOzturk,

I tested the new behavior and found that testNoOverlap is not passing with the new window type. This means that the algorithm behaves worse under no-overlap conditions with the new window type. Since I can't think of a scenario where a non-overlapped analysis is particularly useful I propose the following changes:

  • Explicitly recommend using half-overlapped frames in the algorithm documentation. We can add the following lines:

The recommended pipeline is as follows:

MonoLoader(sampleRate=16000, filename="audio.wav") >> FrameCutter(frameSize=512, hopSize=256, startFromZero=True) >> DiscontinuityDetector(frameSize=512, hopSize=256)

You can check this example.

  • Remove the testNoOverlap, since we no longer recommend this type of analysis.

Any thoughts @dbogdanov ?

@dbogdanov
Copy link
Member

If the frames should be always half-overlapped, why do we need the hopSize parameter?

@palonso
Copy link
Contributor

palonso commented Jun 28, 2023

Normally we use it with half-overlapped frames but the algorithm supports different amounts of overlap.

@dbogdanov
Copy link
Member

I agree with your suggestions. We can either remove or update the expected ground truth for testNoOverlap.

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

Successfully merging this pull request may close these issues.

3 participants