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.

Sign up for GitHub

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

Jump to bottom

Signal.hammingWindow not implemented correctly #4323

Closed
khoin opened this issue Feb 22, 2019 · 16 comments
Closed

Signal.hammingWindow not implemented correctly #4323

khoin opened this issue Feb 22, 2019 · 16 comments
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs.

Comments

@khoin
Copy link
Contributor

khoin commented Feb 22, 2019

The Hamming window is an instance of the Generalized Hamming window with a particular coefficient to cancel out the largest side lobe; it can be seen described here: J.O.S' SASP, or seen implemented elsewhere: MATLAB, Octave, SciPy, etc.

I'll pull the equation from Wikipedia:

Generalized Hamming window family
where a0 = 0.53836... and a1 = 0.46164... yields the Hamming window.

The current SuperCollider code for Signal.hammingWindow is this:

^this.newClear(size).fill(0.5).addSine(1, 0.39, -0.5pi);

which corresponds to a0 = 0.5 and a1 = 0.39, and is given an example here:

(
Signal.hammingWindow(24).plot("SuperCollider default");
Signal.newClear(24).waveFill({ |x|
	0.5 - (0.39*(2*x).cosPi);
}).plot("Explicit Formula Remade");
)

Is this a bug, or is there a rationale behind choosing such coefficient that does not match with the standard definition of Hamming windows? I looked at the blame and that part hasn't been changed for 17 years.

In any case, a window should not have its peak not at 1 either.

A proposed fix:

^this.newClear(size).fill(0.53836).addSine(1, 0.46164, -0.5pi);
@khoin khoin added the bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. label Feb 22, 2019
@nhthn
Copy link
Contributor

nhthn commented Feb 22, 2019

good catch! there is almost certainly no good reason for this. feel free to file a PR to correct the formula.

@khoin
Copy link
Contributor Author

khoin commented Feb 22, 2019 via email

@jamshark70
Copy link
Contributor

My gut feeling, based on some 16-17 years of following list traffic, is that hammingWindow is rarely used. That's not a scientific survey, of course, but if it were very popular, I expect it would have come up on the list from time to time, and I don't recall that it did.

@telephon
Copy link
Member

telephon commented Feb 22, 2019 •
edited
Loading

I know of people using it. In the past, a good strategy has been: rename the current one hammingWindow_old and use hammingWindow for the corrected new one.

@khoin
Copy link
Contributor Author

khoin commented Feb 22, 2019

Terribly sorry, but:

I can't seem to propagate my changes in SCClassLibrary to work at all. I've changed the Signal.sc file in C:\...\SuperCollider-3.10.2\SCClassLibrary\Common\Math\, press save (Ctrl + S), then Recompile Class Library (Ctrl + Shift + L), then I tried Signal.hammingWindow(...) and Signal.hammingWindow_old but they either return the old version or not understood.

DEVELOPING.md didn't seem to help. Do I have to go through the build process for this?

@telephon
Copy link
Member

maybe you haven't made the changes in the library that sclang actually uses. If you open the preferences and open the tab "Interpreter" you may be able to see the path.

@khoin
Copy link
Contributor Author

khoin commented Feb 23, 2019

When I hit Ctrl Shift L, I see the message:
Compiling directory 'C:\Program Files\SuperCollider-3.10.2\SCClassLibrary'

which I've double-checked and made sure that the file I'm making changes to is in that directory. Still, it doesn't work. Very strange.

@khoin
Copy link
Contributor Author

khoin commented Feb 23, 2019

Ach. Nevermind. It's because the directory is protected so all changes were rejected, so I needed admin privileges. I don't think SuperCollider IDE reports when it cannot save a file.

@khoin khoin mentioned this issue Feb 23, 2019
Merged
@telephon
Copy link
Member

telephon commented Feb 23, 2019 •
edited
Loading

I don't think SuperCollider IDE reports when it cannot save a file.

maybe worth another issue!

@jamshark70
Copy link
Contributor

File saving fails silently in Linux too. That's awful. #4325

@khoin
Copy link
Contributor Author

khoin commented Mar 7, 2019

This is awaiting the PR. I'll close this.

@khoin khoin closed this as completed Mar 7, 2019
@mossheim mossheim reopened this Mar 7, 2019
@mossheim
Copy link
Contributor

mossheim commented Mar 7, 2019

Please do not close until PR is merged.

@khoin
Copy link
Contributor Author

khoin commented Mar 7, 2019

Oops. My apologies!

@mossheim
Copy link
Contributor

mossheim commented Mar 7, 2019

np! just helps keep track of what we can actually claim as "fixed" :) we have had seemingly innocuous PRs go stale and never merge in the past

@nhthn
Copy link
Contributor

nhthn commented Jun 9, 2019

fixed in #4324!

@nhthn nhthn closed this as completed Jun 9, 2019
@mossheim
Copy link
Contributor

mossheim commented Jun 9, 2019

Thanks for that @khoin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jamshark70 @nhthn @telephon @khoin @mossheim