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

implements HammingPeriodic and HanningPeriodic #76

Merged
merged 5 commits into from
Aug 21, 2023
Merged

implements HammingPeriodic and HanningPeriodic #76

merged 5 commits into from
Aug 21, 2023

Conversation

gsgou
Copy link
Contributor

@gsgou gsgou commented Aug 18, 2023

closes #75

@gsgou
Copy link
Contributor Author

gsgou commented Aug 18, 2023

@swharden are you gonna have time soon to review and merge this PR?

@swharden
Copy link
Owner

Hi @gsgou

Yes! I'll review this today or tomorrow 👍

@gsgou
Copy link
Contributor Author

gsgou commented Aug 18, 2023

@swharden can you run the test on this once again pls? On my local machine on a mac succeeds.

@gsgou
Copy link
Contributor Author

gsgou commented Aug 18, 2023

@swharden just pushed another commit that fixes a test case as the periodic windows are not symmetric

@gsgou
Copy link
Contributor Author

gsgou commented Aug 20, 2023

@swharden the test should pass now. Roughly when will you have time to review and make a new release of the package?
A quick response will be helpful so i can decide if i should make a release with the fork as submodule or wait a few days more. Tks a lot for the useful lib.

@swharden
Copy link
Owner

Hi @gsgou

I planned to get this done yesterday but got behind, and I'm planning to get this done and released by the end of today 👍

@swharden
Copy link
Owner

swharden commented Aug 21, 2023

planning to get this done and released by the end of today

It's 10:30 pm here (EST) and I'm still underwater some some other projects (including cooking dinner, lol) so it's looking unlikely that this will get finished tonight after all. It's at the top of my stack for tomorrow though, and I look forward to merging #74 and #76 and deploying an updated package as soon as possible 👍

EDIT: By "top of my stack for tomorrow" I mean after I get home from my 9-5 and eat dinner 😅

@swharden
Copy link
Owner

Note to self, this is what these new windows look like

Hamming Hamming Periodic
image image
Hanning Hanning Periodic
image image

@swharden
Copy link
Owner

swharden commented Aug 21, 2023

@gsgou I notice that scipy accepts a symmetrical argument (defaults to True), and if set to False it generates a non-symmetrical periodic window.

Rather than add two new windows, we could modify IWindow to add a similar argument so users can opt-in to get the periodic feature for all existing windows. What do you think of this idea?

namespace FftSharp
{
public interface IWindow
{
/// <summary>
/// Generate this window as a new array with the given length.
/// Normalizing will scale the window so the sum of all points is 1.
/// </summary>
double[] Create(int size, bool normalize = false);
/// <summary>
/// Return a new array where this window was multiplied by the given signal.
/// Normalizing will scale the window so the sum of all points is 1 prior to multiplication.
/// </summary>
double[] Apply(double[] input, bool normalize = false);
/// <summary>
/// Modify the given signal by multiplying it by this window IN PLACE.
/// Normalizing will scale the window so the sum of all points is 1 prior to multiplication.
/// </summary>
void ApplyInPlace(double[] input, bool normalize = false);
/// <summary>
/// Single word name for this window
/// </summary>
string Name { get; }
/// <summary>
/// A brief description of what makes this window unique and what it is typically used for.
/// </summary>
string Description { get; }
}
}

@swharden
Copy link
Owner

we could modify IWindow to add a similar argument

To keep things rolling I'll make an IPeriodicWindow and apply it to Hamming and Hanning and we can decide whether to bake these functions into IWindow later 👍

@gsgou
Copy link
Contributor Author

gsgou commented Aug 21, 2023

@gsgou I notice that scipy accepts a symmetrical argument (defaults to True), and if set to False it generates a non-symmetrical periodic window.

Rather than add two new windows, we could modify IWindow to add a similar argument so users can opt-in to get the periodic feature for all existing windows. What do you think of this idea?

You are using an interface common for all the windows. But not all windows take an argument for symmetrical, hamming, hanning, cosinus etc do but not the others. Therefore i used an explicit name but you may want to refactor.

@swharden
Copy link
Owner

IPeriodicWindow and apply it to Hamming and Hanning

I started doing this and it got messy 😝

Therefore i used an explicit name but you may want to refactor.

I think that's the way to go! I'll merge shortly and issue a new release tonight 👍

Thanks again for these PRs!

@gsgou
Copy link
Contributor Author

gsgou commented Aug 21, 2023

Your welcome. Tks for providing this tool ;)

@swharden swharden merged commit 58379f1 into swharden:main Aug 21, 2023
1 check passed
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.

add HammingPeriodic, HanningPeriodic
2 participants