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

eval_mpfr method added #456

Merged
merged 4 commits into from Jun 5, 2015
Merged

eval_mpfr method added #456

merged 4 commits into from Jun 5, 2015

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented May 25, 2015

No description provided.

@certik
Copy link
Contributor

certik commented May 25, 2015

Looks good. I think we should also set the MPFR precision in the upper
level function and use this precision throughout.

Sent from my mobile phone.
On May 25, 2015 6:18 AM, "Isuru Fernando" notifications@github.com wrote:


You can view, comment on, or merge this pull request online at:

#456
Commit Summary

  • eval_mpfr method added

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#456.

@isuruf
Copy link
Member Author

isuruf commented May 25, 2015

I didn't set it because the user could give the precision by initializing mpfr_t in eval_mpfr(mpfr_t result, const Basic &b, mpfr_rnd_t rnd); with the necessary precision.

@certik
Copy link
Contributor

certik commented May 25, 2015

Ah, yes, that's the way to go. Add some tests with various precisions, to make this clear. Now when we have this, we can for example take some more difficult example (I know there is one, where the answer looks like an integer, or 167.99999 to be precise, and then after like 15 digits you get something other than 9). We can evaluate with our eval_double and then check the answer with eval_mpfr as well as show that higher precision provides more accurate result. I'll try to lookup the example.

@Sumith1896
Copy link
Contributor

Does this come as a cmake option?

@isuruf
Copy link
Member Author

isuruf commented Jun 3, 2015

@certik, I added a test. Is this what you had in mind? (I'll add more tests)
@Sumith1896 yes, -DWITH_MPFR=yes to enable it or if you have arb, just -DWITH_ARB=yes

@certik
Copy link
Contributor

certik commented Jun 3, 2015

@isuruf Yes, exactly. I think you need to merge with master, there are some conflicts.

@isuruf
Copy link
Member Author

isuruf commented Jun 4, 2015

Merged with master and improved the comments. Please check if I missed something in the comments.

@certik
Copy link
Contributor

certik commented Jun 4, 2015

This is very good. +1 to merge.

@certik certik changed the title [WIP] eval_mpfr method added eval_mpfr method added Jun 4, 2015
isuruf added a commit that referenced this pull request Jun 5, 2015
@isuruf isuruf merged commit d05b4d1 into symengine:master Jun 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants