fix(spectralflux): use magnitude spectrum rather than real component of complex spectrum #1076
Conversation
A constant exists in the formula for converting between hz and the mel scale, which Meyda has had set to 1125. The mel-scale is not standardised, but it seems that the most popular formula comes from "Speech communication: human and machine" by Douglas O'Shaughnessy. That formula has the constant as 1127. The issue mentioned below was opened, and suggests that we converge on O'Shaughnessy's constant. This commit implements that change. BREAKING CHANGE: A change in how Meyda performs frequency to mel scale conversion will result in different mel-values between this and previous versions of Meyda. fix #458
…enance BREAKING CHANGE: remove nodes 10 and 13 fix #842
Also document the upper bound of flux as the square root of the length of the buffers, and if only one buffer is given, return 0
also minor corrections in spectral flux and tests for that re #1074
…of complex spectrum re #1074
I didn't realize magnitude spectrum and amplitude spectrum were the same thing
|
I've now fixed it and it's using the existing amplitude spectrum (which I believe is the same as magnitude spectrum) rather than calculating the magnitude spectrum separately. |
| }); | ||
|
|
||
| expect(en).toEqual(2.1288412531209104e-8); | ||
| expect(en).toEqual(2.8276224855864956e-8); |
There was a problem hiding this comment.
Should we be using toBeCloseTo instead? This number is supposed to be 0, but due to Javascript™️ it isn't.
There was a problem hiding this comment.
Yes, and we should also be testing with a real set of different signals, rather than just the same one twice. I'll add that.
| }); | ||
|
|
||
| expect(en).toEqual(1.03425562865083e-7); | ||
| expect(en).toEqual(6.8073007097613e-8); |
| ); | ||
| const normalizedMagnitudeSpectrum = normalizeToOne(ampSpectrum); | ||
| const previousNormalizedMagnitudeSpectrum = | ||
| normalizeToOne(previousAmpSpectrum); |
There was a problem hiding this comment.
I wonder if this is correct. Should they be normalised between themselves? Should they be normalised at all? I am not sure normalising each one to 1 separately makes sense. Can you share the source this was taken from?
FWIW, Matlab implements it without normalisation, it seems.
There was a problem hiding this comment.
I'm not sure - I added normalization because wikipedia implies that only some implementations do not normalize. I think it's useful because it makes this feature purely about the spectral content, and resilient to overall volume changes, which can be measured much better with rms and loudness. But, I haven't gone looking for other implementations yet.
src/extractors/positiveFlux.ts
Outdated
| import { normalizeToOne } from "../utilities"; | ||
|
|
||
| export default function ({ | ||
| complexSpectrum, | ||
| previousComplexSpectrum, | ||
| ampSpectrum, | ||
| previousAmpSpectrum, | ||
| }: { | ||
| complexSpectrum: { real: number[]; imag: number[] }; | ||
| previousComplexSpectrum: { real: number[]; imag: number[] }; | ||
| ampSpectrum: Float32Array; | ||
| previousAmpSpectrum: Float32Array; | ||
| }): number { | ||
| if (!previousComplexSpectrum) { | ||
| if (!previousAmpSpectrum) { | ||
| return 0; | ||
| } | ||
|
|
||
| const normalizedRealComponent = normalizeToOne(complexSpectrum.real); | ||
| const previousNormalizedRealComponent = normalizeToOne( | ||
| previousComplexSpectrum.real | ||
| ); | ||
| const normalizedMagnitudeSpectrum = normalizeToOne(ampSpectrum); | ||
| const previousNormalizedMagnitudeSpectrum = | ||
| normalizeToOne(previousAmpSpectrum); | ||
|
|
||
| let sf = 0; | ||
| for (let i = 0; i < normalizedRealComponent.length; i++) { | ||
| for (let i = 0; i < normalizedMagnitudeSpectrum.length; i++) { | ||
| let x = | ||
| Math.abs(previousNormalizedRealComponent[i]) - | ||
| Math.abs(normalizedRealComponent[i]); | ||
| Math.abs(normalizedMagnitudeSpectrum[i]) - | ||
| Math.abs(previousNormalizedMagnitudeSpectrum[i]); | ||
| sf += Math.pow(Math.max(x, 0), 2); |
There was a problem hiding this comment.
Actually, why do we have both a positiveFlux and a spectralFlux implementations?
There was a problem hiding this comment.
I thought we could separate them as features rather than add another global flag about ignoring decreases in magnitude between buffers that gets passed down, but I'm open to either way. This way there's code duplication, but I'm not too worried about that.
|
|
||
| export function magnitudeForComplexSpectrum(complexSpectrum) { | ||
| return complexSpectrum.real.map((real, i) => { | ||
| const imag = complexSpectrum.imag[i]; | ||
| return Math.sqrt(Math.pow(real, 2) + Math.pow(imag, 2)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Don't we have this somewhere already? Many feature extractors already use the amplitude spectrum AFAIK.
There was a problem hiding this comment.
Yes - it's here - and even in this PR we actually use that implementation, not this one. This function is from before I remembered that magnitude spectrum and amplitude spectrum are the same.
In that part of the source, the calculation is an inline snippet in a function for preparing the spectrum, and it only calculates the lower half of the buffer (which is good). But when I swapped it out for half the result of this function, I got a different result, so I wanted to keep this unused function around and debug it later.
|
Can we maybe test this with a file that has clear onsets? Like a drum beat. |
This runs spectral flux on the magnitude spectrum, both for
spectralFluxandpositiveFlux.Of course, I'm an idiot and didn't remember that amp spectrum is the same as magnitude spectrum and wrote a new thing, rather than use our existing amp spectrum, so I'll fix that.
For positiveFlux, I think I've done something wrong. The following is a graph of spectral flux over the entire course of the elvis song on the meyda demo page. The big flux peak is at the end of the song, rather than the start. I guess maybe there's a click there and that could've been fine. I've checked the maths but like, who knows.
cc @nevosegal
re #1074