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

deceiving name of fft_overlap variable #72

Open
jerabaul29 opened this issue Jan 9, 2023 · 0 comments
Open

deceiving name of fft_overlap variable #72

jerabaul29 opened this issue Jan 9, 2023 · 0 comments
Labels
quirk wontfix This will not be worked on

Comments

@jerabaul29
Copy link
Owner

jerabaul29 commented Jan 9, 2023

// we use FFT of length 2048; smaller is bad for frequency resolution
constexpr size_t fft_length {2048};

vs.

// use a 75% overlap in the Welch computation
constexpr size_t fft_overlap {512};

vs.

// prepare for the next segment
start_current_segment += fft_overlap;
end_current_segment += fft_overlap;

Note that this well corresponds to a 75% overlap as indicated in the comment of fft_overlap definition, but the fft_overlap name is poorly chosen; really it is the fft_nonoverlap, not the fft_overlap that is being defined (the relation between the two being, fft_nonoverlap = fft_length - fft_overlap). So, while the logics are correct / correspond to the 75% overlap comment, the naming of the variables is very poor / misleading.

Many thanks to @gauteh for pointing this :) . This does not affect the correctness of the computations run, so wont fix for now, but keeping this issue open so that it is visible.

@jerabaul29 jerabaul29 added wontfix This will not be worked on quirk labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quirk wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

1 participant