-
Notifications
You must be signed in to change notification settings - Fork 104
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
fix(spectralflux): use magnitude spectrum rather than real component of complex spectrum #1076
base: v6
Are you sure you want to change the base?
Changes from all commits
4b1a068
ef97e3f
12e27ea
c16427c
715689c
d8d6d0e
303cce9
82b2731
b258456
c57f008
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ on: | |
push: | ||
branches: | ||
- main | ||
- v6 | ||
|
||
jobs: | ||
build: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import TestData from "../TestData"; | ||
|
||
// Setup | ||
var positiveFlux = require("../../dist/node/extractors/positiveFlux"); | ||
|
||
describe("positiveFlux", () => { | ||
test("should return correct Positive Flux value", (done) => { | ||
var en = positiveFlux({ | ||
ampSpectrum: TestData.VALID_AMPLITUDE_SPECTRUM.map((e) => e * 0.8), | ||
previousAmpSpectrum: TestData.VALID_AMPLITUDE_SPECTRUM, | ||
}); | ||
|
||
expect(en).toEqual(2.8276224855864956e-8); | ||
|
||
done(); | ||
}); | ||
|
||
test.skip("should throw an error when passed an empty object", (done) => { | ||
try { | ||
var en = positiveFlux({}); | ||
} catch (e) { | ||
done(); | ||
} | ||
}); | ||
|
||
test.skip("should throw an error when not passed anything", (done) => { | ||
try { | ||
var en = positiveFlux(); | ||
} catch (e) { | ||
done(); | ||
} | ||
}); | ||
|
||
test.skip("should throw an error when passed something invalid", (done) => { | ||
try { | ||
var en = positiveFlux({ signal: "not a signal" }); | ||
} catch (e) { | ||
done(); | ||
} | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import TestData from "../TestData"; | ||
|
||
// Setup | ||
var spectralFlux = require("../../dist/node/extractors/spectralFlux"); | ||
|
||
describe("spectralFlux", () => { | ||
test("should return correct Spectral Flux value", (done) => { | ||
var en = spectralFlux({ | ||
ampSpectrum: TestData.VALID_AMPLITUDE_SPECTRUM, | ||
previousAmpSpectrum: TestData.VALID_AMPLITUDE_SPECTRUM.map( | ||
(e) => e * 0.8 | ||
), | ||
}); | ||
|
||
expect(en).toEqual(6.8073007097613e-8); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
|
||
done(); | ||
}); | ||
|
||
test.skip("should throw an error when passed an empty object", (done) => { | ||
try { | ||
var en = spectralFlux({}); | ||
} catch (e) { | ||
done(); | ||
} | ||
}); | ||
|
||
test.skip("should throw an error when not passed anything", (done) => { | ||
try { | ||
var en = spectralFlux(); | ||
} catch (e) { | ||
done(); | ||
} | ||
}); | ||
|
||
test.skip("should throw an error when passed something invalid", (done) => { | ||
try { | ||
var en = spectralFlux({ signal: "not a signal" }); | ||
} catch (e) { | ||
done(); | ||
} | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { normalizeToOne } from "../utilities"; | ||
|
||
export default function ({ | ||
ampSpectrum, | ||
previousAmpSpectrum, | ||
}: { | ||
ampSpectrum: Float32Array; | ||
previousAmpSpectrum: Float32Array; | ||
}): number { | ||
if (!previousAmpSpectrum) { | ||
return 0; | ||
} | ||
|
||
const normalizedMagnitudeSpectrum = normalizeToOne(ampSpectrum); | ||
const previousNormalizedMagnitudeSpectrum = | ||
normalizeToOne(previousAmpSpectrum); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
let sf = 0; | ||
for (let i = 0; i < normalizedMagnitudeSpectrum.length; i++) { | ||
let x = | ||
Math.abs(normalizedMagnitudeSpectrum[i]) - | ||
Math.abs(previousNormalizedMagnitudeSpectrum[i]); | ||
sf += Math.pow(Math.max(x, 0), 2); | ||
nevosegal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return Math.sqrt(sf); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,26 @@ | ||
// This file isn't being typechecked at all because there are major issues with it. | ||
// See #852 for details. Once that's merged, this file should be typechecked. | ||
// @ts-nocheck | ||
import { normalizeToOne } from "../utilities"; | ||
|
||
export default function ({ | ||
signal, | ||
previousSignal, | ||
bufferSize, | ||
ampSpectrum, | ||
previousAmpSpectrum, | ||
}: { | ||
signal: Float32Array; | ||
previousSignal: Float32Array; | ||
bufferSize: number; | ||
ampSpectrum: Float32Array; | ||
previousAmpSpectrum: Float32Array; | ||
}): number { | ||
if (typeof signal !== "object" || typeof previousSignal != "object") { | ||
throw new TypeError(); | ||
if (!previousAmpSpectrum) { | ||
return 0; | ||
} | ||
|
||
const normalizedMagnitudeSpectrum = normalizeToOne(ampSpectrum); | ||
const previousNormalizedMagnitudeSpectrum = | ||
normalizeToOne(previousAmpSpectrum); | ||
|
||
let sf = 0; | ||
for (let i = -(bufferSize / 2); i < signal.length / 2 - 1; i++) { | ||
x = Math.abs(signal[i]) - Math.abs(previousSignal[i]); | ||
sf += (x + Math.abs(x)) / 2; | ||
for (let i = 0; i < normalizedMagnitudeSpectrum.length; i++) { | ||
let x = | ||
normalizedMagnitudeSpectrum[i] - previousNormalizedMagnitudeSpectrum[i]; | ||
sf += Math.pow(x, 2); | ||
} | ||
|
||
return sf; | ||
return Math.sqrt(sf); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,13 +93,15 @@ export function mean(a) { | |
); | ||
} | ||
|
||
const MEL_CONSTANT = 1127; | ||
|
||
function _melToFreq(melValue) { | ||
var freqValue = 700 * (Math.exp(melValue / 1125) - 1); | ||
var freqValue = 700 * (Math.exp(melValue / MEL_CONSTANT) - 1); | ||
return freqValue; | ||
} | ||
|
||
function _freqToMel(freqValue) { | ||
var melValue = 1125 * Math.log(1 + freqValue / 700); | ||
var melValue = MEL_CONSTANT * Math.log(1 + freqValue / 700); | ||
return melValue; | ||
} | ||
|
||
|
@@ -270,3 +272,10 @@ export function frame(buffer, frameLength, hopLength) { | |
.fill(0) | ||
.map((_, i) => buffer.slice(i * hopLength, i * hopLength + frameLength)); | ||
} | ||
|
||
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)); | ||
}); | ||
} | ||
Comment on lines
+275
to
+281
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we have this somewhere already? Many feature extractors already use the amplitude spectrum AFAIK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.