Phasor tests fail

[sebpiq]

At some point, the accumulated phase reaches something like 21.999999275431037 which should correspond to phase === 1, but what happens instead is that this is rounded to 22, and we get phase === 22 % 1 # -> 0.

In the test case generated with Pd, the phase is 1. Which one is right?

[sebpiq]

Probably what would be needed is to test the modulo of the diff, and not the diff itself. e.g. :

val[i] ~ val[j] if (val[i] - val[j]) % 32765 < tolerance

[jussi-kalliokoski]

Hmm, floating points, they're so great... With the current implementation the phase never reaches full power, which, if you think about it, is an obvious error. The correct implementation would be that the first sample of the cycle is always zero and also one (if our period is an integer of samples); However, since a linear function can't be both at the same time, we'll just have to never hit one, or not start from zero. Looks like Pd has gone with not starting from zero. I have no personal preference, but never hitting one is significantly easier to implement.

Changing the tests like you described sounds reasonable to me.

BTW, we might want to consider using relative error in our tests rather than absolute error, as in

relativeError = resultValue / referenceValue - 1

That's IMHO a better tool for comparing values than absolute error given that you can keep the same threshold regardless of the magnitude of the values.

[sebpiq]

Yeah ... I think chai-stats doesn't fit the bill. it's too simplistic. Relative error sounds good. Pd both starts from 0 and hits 1 (see here : https://raw.github.com/sebpiq/dsp-test-files/master/test-files/waveforms/phasor-440-hz.json). But it's true that in our case it's just much simpler to not hit 1.

I have changed the phasor algorithm to start from 0, but it requires 2 buffers instead of 1 (buffer increments which wasn't necessary before) :

Phasor.prototype.defaults = {
    frequency: 440,
    initialPhase: 0
};

/**
 * Calculates the phase based on the frequency.
 *
 * @param {Float32Array} phase The array to write the phase to.
*/
Phasor.prototype.process = function (phase, frequency) {
    frequency = frequency || this.parameters.frequency
    var offset = this.blockSize - Tools.calculateOffset(
        phase,
        frequency || frequency
    );
    frequency = Tools.offset(frequency, offset);

    var increments = new Float32Array(phase.length);
    ArrayMath.add(increments, frequency, phase); // TODO Why + phase? Instead of just increments[i] = frequency[i]
    ArrayMath.mul(increments, 1 / this.sampleRate, increments);

    var lastPhase = phase[0] = this.lastPhase;
    for ( var i = 0, length = (phase.length - 1); i < length; i++ ) {
        phase[i + 1] = lastPhase = lastPhase + increments[i];
    }
    ArrayMath.fract(phase, phase);
    this.lastPhase = lastPhase + increments[length];
};

what do you think?

[jussi-kalliokoski]

Pd both starts from 0 and hits 1

Yes, so it seems. But looking at that data, there's only one 32767 and one 0, so it's hard to say if it only starts from zero the first time. I doubt zero would follow one, since they're essentially the same phase for most algorithm implenentations, hence you would get a slight glitch if you have the same phase for two consequent samples (unless it's a reaaaaallly slow frequency).

[jussi-kalliokoski]

Not hitting one also makes it simpler for example for sampler playback, e.g.

value = interpolate(buffer: buffer, samplePosition: phase * buffer.length)

Where you would have an out-of-bounds read where phase is one.

If you designed for reaching zero, I don't want to even think about how to make that implementation not glitch since if you just did samplePosition: phase * (buffer.length - 1) you'd miss the interpolation between the last and the first sample.

[jussi-kalliokoski]

what do you think?

Could we go simpler? e.g.

Phasor.prototype.process = function (phase, frequency) {
    frequency = frequency || this.parameters.frequency;
    var offset = this.blockSize - Tools.calculateOffset(
        phase,
        frequency || frequency
    );
    frequency = Tools.offset(frequency, offset);

    phase.set(frequency);
    ArrayMath.mul(phase, 1 / this.sampleRate, phase);

    phase[0] += this.lastPhase;
    for ( var i = 1; i < phase.length; i++ ) {
        phase[i] += phase[i - 1];
    }
    ArrayMath.fract(phase, phase);
    this.lastPhase = phase[i - 1];
};

Doesn't this do the same thing without the need for the extra buffer?

[sebpiq]

yes! Great :) I'll implement it along with better approximation functions for tests and pull request.

[sebpiq]

Hmmm actually this doesn't work either. 1- you need to handle case where frequency is a number, 2- this doesn't start from 0. This phasor starts to annoy me.

[sebpiq]

possibly fixed by 137d041