Skip to content
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

arm_cos_f32 with around -pi/2 #267

Closed
idt12312 opened this issue Oct 26, 2017 · 9 comments
Closed

arm_cos_f32 with around -pi/2 #267

idt12312 opened this issue Oct 26, 2017 · 9 comments

Comments

@idt12312
Copy link

idt12312 commented Oct 26, 2017

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.

/* Special case for small negative inputs */

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
Copy link
Member

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
Copy link
Author

idt12312 commented Oct 26, 2017

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
Copy link
Contributor

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

@idt12312
Copy link
Author

idt12312 commented Nov 8, 2017

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

@llefaucheur
Copy link
Contributor

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
Copy link
Author

idt12312 commented Dec 3, 2017

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

@madcowswe
Copy link
Contributor

madcowswe commented Sep 27, 2018

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
Copy link
Contributor

@idt12312 Please see the fix in #439

@idt12312
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants