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

fix trig function having a multiple of pi/3, p/4, pi/6, pi/12; fixes … #1051

Merged
merged 6 commits into from Sep 16, 2016

Conversation

rwst
Copy link
Contributor

@rwst rwst commented Jul 30, 2016

#1035

@rwst
Copy link
Contributor Author

rwst commented Jul 30, 2016

The problem (apart from the incompleteness) was that the original author returned the wrong expression for unhandled cases in eval()s rarg. Returning the right thing however was impossible unless the canonical check was made less stringent, else an infinite loop or a memleak occurred (because is_canonical bailed out where it should not).

@certik
Copy link
Contributor

certik commented Jul 31, 2016

Thanks @rwst for fixing it! I need to go over your code more carefully.

@isuruf
Copy link
Member

isuruf commented Sep 11, 2016

@rwst, I have some improvements to this PR. Can you enable Allow edits from maintainers for this PR? (There should be a tick box in the right side bar)

@rwst
Copy link
Contributor Author

rwst commented Sep 12, 2016

Please go ahead.

@isuruf
Copy link
Member

isuruf commented Sep 12, 2016

@certik, @rwst, can you review?

@isuruf
Copy link
Member

isuruf commented Sep 12, 2016

Change I've made is to reduce sin(r*pi + x) to the first quadrant for any r rational number.

@certik
Copy link
Contributor

certik commented Sep 12, 2016

I think that looks good to me.

How are benchmarks with trigonometric functions affected?

@isuruf
Copy link
Member

isuruf commented Sep 12, 2016

We don't have any that uses the code paths changed in this PR. Only simple arguments like x are used for trig functions

@certik
Copy link
Contributor

certik commented Sep 12, 2016

I see, thanks. +1 to merge then.

@isuruf isuruf merged commit 827d1f5 into symengine:master Sep 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants