ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

arm_cos_f32 with around -pi/2 #267

Closed idt12312 closed 6 years ago

idt12312 commented 6 years ago

Problem

I found a result value from arm_cos_f32 is wrong in a particular situation.

The problem occurred when I call the function with a parameter around -pi/2. Concretely, when the parameter is 0xbfc90fdc or 0xbfc90fdd (this value means -pi/2 in 32bit float), this problem occurred.

Possible Cause

I think this problem is related to a past bug of arm_sin_f32. According to the following change log, a bug of arm_sin_f32 was fixed at Version 1.4.10. http://www.keil.com/pack/doc/CMSIS/DSP/html/ChangeLog_pg.html

I think added lines to fix this bug are here. https://github.com/ARM-software/CMSIS_5/blob/d05a7dc7e9fcf0e013b4494cde905ae5f8065a59/CMSIS/DSP/Source/FastMathFunctions/arm_sin_f32.c#L81

Although the implementation of arm_cos_f32 is very similar to arm_sin_f32 and a same bug can occur in arm_sin_f32 cos, changes have been made only for arm_sin_f32.

JonatanAntoni commented 6 years ago

Hi @idt12312,

thanks for letting us know.

Is it feasible to to ask you for some more details to reproduce the problem and validate a fix? I guess you call arm_cos_f32(0xbfc90fdc). Which result do you get and which one do you expect?

Best, Jonatan

idt12312 commented 6 years ago

Hi @JonatanAntoni , thanks for your response.

Concretely, when the parameter is 0xbfc90fdc or 0xbfc90fdd (this value means -pi/2 in 32bit float), this problem occurred.

This means as follows.

uint32_t x_bin = 0xbfc90fdc;
float *x = (float*)&x_bin;  // *x is -pi/2 in float
float cos_result = arm_cos_f32(*x);

This cos_result should be about 0.0 (=cos(-pi/2)), but this is over 1.0. In my case, cos_result was 2pi.

llefaucheur commented 6 years ago

Hi, From API's documentation, the parameter x must stay in the range [0 .. 2.Pi] Regards, Laurent.

idt12312 commented 6 years ago

@llefaucheur I have never known that. Where can I see the API's documentation?

llefaucheur commented 6 years ago

You are true the comments in the code (not exactly reproduced in the API) are not as strong as it should (https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/DSP/Source/FastMathFunctions/arm_cos_f32.c#L80 : "Scale the input to [0 1] range from [0 2*PI] " without telling to avoid data out of this range. We will correct this quickly. Concerning the behavior at -0.25x(2.Pi) which works by chance up to -0.24999 : this is related to the reading of the coefficient's table used for Sin(x) which is identical to Cos(x) shifted by Pi/2. Thank you again for your contribution to improving the documentation.

idt12312 commented 6 years ago

@llefaucheur @JonatanAntoni Sorry for late reply. I have understood the use of this function. Thank you for your support.

madcowswe commented 6 years ago

Hi, From API's documentation, the parameter x must stay in the range [0 .. 2.Pi] Regards, Laurent.

I don't think this makes sense. The test data, and the examples include plenty of negative numbers.

Although the implementation of arm_cos_f32 is very similar to arm_sin_f32 and a same bug can occur in arm_sin_f32 cos, changes have been made only for arm_sin_f32.

I think this is exactly right. The special case implemented in arm_sin_f32 should be carried over to arm_cos_f32.

We can see that in the top of the test data there is already a special entry of -1.5E-07, seemingly to verify the arm_sin_f32. We should similarly add -1.5707965 that @idt12312 identified, to check correct operation of arm_cos_f32.

EDIT: Here you can see the otherwise correct operation, including negative inputs, of arm_cos_f32 with the spike at -pi/2:

plot

madcowswe commented 5 years ago

@idt12312 Please see the fix in #439

idt12312 commented 5 years ago

@madcowswe Thank you for letting me know. This seems to be fundamental solution rather than just modifing document. I hope to use merged code.