M_NORMALIZE_RADIANS doesn't work

[westonal]

Claims to normalize to [-pi..pi]

Bad example:

M_NORMALIZE_RADIANS(3.141593)=-21.991148

In this PR #117 I have alternate function.

[kfitch]

I noticed that p_cos_f32 uses M_NORMALIZE_RADIANS, but p_sin_f32 does not.

[mateunho]

I think they both should not use it, since functions are meant to calculate value for input from range: 0 to 2pi (as stated here)

CC @aolofsson @olajep

[westonal]

@mateunho Yeah I saw that. I think that should be revisited. Any client code using the functions would likely have to normalise the angles before calling and so we might as well do it, well and fast.

[westonal]

I believe we can do this with a few bitwise operations/shifts and two multiplies: (from PR #117)

float normalizeRadiansToPlusMinusM_PI(float radians) {
    unsigned int *radiansBits = (unsigned int *)&radians;
    unsigned int signBit = *radiansBits & 0x80000000u;
    *radiansBits = *radiansBits ^ signBit; //remove any negative bit

    int revolutions = (int) (radians * M_1_PI) + 1;
    revolutions = revolutions >> 1; // div 2

    radians = radians - revolutions * (2 * M_PI); //subtract whole revolutions, now in range [-pi..pi]

    *radiansBits = *radiansBits ^ signBit; //if was negative, flip negative bit
    return radians;
}

[mateunho]

Those functions must have well defined domain of operation. For example: what if client code passes M_MAXFLOAT? Should we normalize argument or return error or do the math?
Current approach would do the math. However it's more intuitive to check the domain and return an error (or just return from function). But I think that normalizing argument is the worst possible option, see for example ref

[westonal]

What am I looking at on that link?

[westonal]

@mateunho

I'm just saying passing -2*pi should not be an issue the client code should have to worry about. It's outside the [0..2PI] range, but so what, it's perfectly valid for sin and cos and is confusing to have this arbitrary range imposed.

And yes for M_MAXFLOAT, the function should normalise and do the math. Why not?

As for throwing errors, that goes against the 'Super fast but no "belt AND suspenders"' approach of the project.

[mateunho]

@westonized

And yes for M_MAXFLOAT, the function should normalise and do the math. Why not?

M_MAXFLOAT server as an infinity value. And there's no such thing as sin(infinity). It simply does not exist.

[westonal]

@mateunho Why are the normal IEEE-754 infinities not used?

But I repeat, wouldn't checking go against that design goal?

[kfitch]

On a related note, the current implementations of sin and cos are inaccurate outside of about -pi to pi (and not even that great near +/0 pi). sin is off by about 0.6 at 2*pi, but only off by 2e-5 at pi. cos is worse. Unfortunately the test vectors currently used for the trig functions seem to be in the range [-1,1], where the approximations are quite good.

I did a quick check where I expanded the range of the test vector for sincos to be [-pi, pi]. cos fails with a value of -1.001829 at -3.141593. That seems pretty bad to me.

[westonal]

@kfitch yes I noticed that. I think that they are based on a series calculation, with not enough iterations. Buy that's not generally regarded as a good way to do it anyway. For one it's all floating point multiplies.

sincos is currently calling them where as I think that actually it would be better to call my CORDIC sincos (PR #117 ) from sin and cos and throw away the half you are not interested in. Certainly worth a benchmark test.