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

Changed euler constant to euler number in eval_mpfr and eval_mpc #761

Merged
merged 3 commits into from Jan 27, 2016

Conversation

Projects
None yet
5 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Jan 16, 2016

Fix for #701 .
From euler constant(0.577) to euler's number(2.718)
@isuruf @abhinavagarwal07

@@ -232,15 +232,23 @@ class EvalMPCVisitor : public BaseVisitor<EvalMPCVisitor> {
} else if (x.__eq__(*E)) {
mpfr_t t;
mpfr_init2(t, mpc_get_prec(result_));
mpfr_const_euler(t, rnd_);
mpfr_t one_;
mpfr_init(one_);

This comment has been minimized.

@isuruf

isuruf Jan 17, 2016

Member

You are initializing one_ to default precision which is not the same as the precision of result_

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 17, 2016

Contributor

Changed precision to match with result_

mpc_set_fr(result_, t, rnd_);
mpfr_clear(t);
mpfr_clear(one_);

This comment has been minimized.

@isuruf

isuruf Jan 17, 2016

Member

Instead of having t and one_ here, use one variable t as it is enough in this case.

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 17, 2016

Contributor

Aren't both variables required? since we have mpfr_exp(t,one_,rnd_);

This comment has been minimized.

@isuruf

isuruf Jan 17, 2016

Member

You can do mpfr_exp(t, t, rnd_)

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 17, 2016

Contributor

Do you mean this?

mpfr_t t;
mpfr_init2(t, mpc_get_prec(result_));
mpfr_set_si(t,1,rnd_);
mpfr_exp(t,t, rnd_);
@@ -19,6 +19,7 @@ class EvalMPFRVisitor : public BaseVisitor<EvalMPFRVisitor> {
protected:
mpfr_rnd_t rnd_;
mpfr_ptr result_;
mpfr_ptr one_;

This comment has been minimized.

@isuruf

isuruf Jan 17, 2016

Member

This is not needed.

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 17, 2016

Contributor

Changed this to local variable.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 17, 2016

Test are failing.I guess they were set according to euler constant.I'm working on it.

@@ -213,15 +213,20 @@ class EvalMPFRVisitor : public BaseVisitor<EvalMPFRVisitor> {
};

void bvisit(const Constant &x) {

mpfr_ptr one_;

This comment has been minimized.

@isuruf

isuruf Jan 17, 2016

Member

This should be mpfr_t

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 17, 2016

Contributor

Done

} else if (x.__eq__(*EulerGamma)) {
mpfr_const_euler(result_, rnd_);
mpfr_exp(result_,one_, rnd_);
} else {
throw std::runtime_error("Constant " + x.get_name() + " is not implemented.");

This comment has been minimized.

@isuruf

isuruf Jan 17, 2016

Member

If an error is thrown here, one_ is not cleared.

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 18, 2016

Contributor

Though this is not causing the error I have cleared it after line 223 in the else if.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 18, 2016

@isuruf I don't know why but Line 54 of test_eval_mpfr.cpp is failing. I changed 0.00000000490154 to 1 being sure that it will definitely return false in the function call but I still got the same error.Don't know why mpfr_cmp_d is not functioning properly.

} else if (x.__eq__(*EulerGamma)) {
mpfr_const_euler(result_, rnd_);
mpfr_exp(result_,one_, rnd_);

This comment has been minimized.

@isuruf

isuruf Jan 19, 2016

Member

Changes should be done only to E, not to EulerGamma

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 19, 2016

Contributor

Thanxx!!Fixed.


mpfr_t one_;
mpfr_init2(one_,mpfr_get_prec(result_));
mpfr_set_d(one_,1,rnd_);

This comment has been minimized.

@isuruf

isuruf Jan 19, 2016

Member

Move this block inside the else if

@@ -232,13 +232,15 @@ class EvalMPCVisitor : public BaseVisitor<EvalMPCVisitor> {
} else if (x.__eq__(*E)) {
mpfr_t t;
mpfr_init2(t, mpc_get_prec(result_));
mpfr_const_euler(t, rnd_);
mpfr_set_si(t,1,rnd_);

This comment has been minimized.

@isuruf

isuruf Jan 19, 2016

Member

Add space after ,

mpc_set_fr(result_, t, rnd_);
mpfr_clear(t);
} else if (x.__eq__(*EulerGamma)) {
mpfr_t t;
mpfr_init2(t, mpc_get_prec(result_));
mpfr_const_euler(t, rnd_);
mpfr_set_si(t,1,rnd_);
mpfr_exp(t,t, rnd_);

This comment has been minimized.

@isuruf

isuruf Jan 19, 2016

Member

Remove this change as well

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 19, 2016

Contributor

Done

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:fix-euler-number branch 2 times, most recently from 7806a77 to d102856 Jan 19, 2016

} else if (x.__eq__(*EulerGamma)) {
mpfr_const_euler(result_, rnd_);
mpfr_const_euler(result_,rnd_);

This comment has been minimized.

@isuruf

isuruf Jan 19, 2016

Member

Space after comma

mpfr_const_euler(result_, rnd_);
mpfr_t one_;
mpfr_init2(one_, mpfr_get_prec(result_));
mpfr_set_d(one_, 1, rnd_);

This comment has been minimized.

@isuruf

isuruf Jan 19, 2016

Member

This should be mpfr_set_ui

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 19, 2016

Contributor

Does it matter if its long int or double ?

This comment has been minimized.

@isuruf

isuruf Jan 19, 2016

Member

Precision of double is 53 whereas the precision of result_ can be much larger than that. I'm not sure of the internals of MPFR and whether having 1 as a double matters, but it's always better to use exact numbers.

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 19, 2016

Contributor

I agree.
Updated.

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Jan 20, 2016

Looks good to me. +1 to merge

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 20, 2016

Appveyor build needs to be restarted.

@Sumith1896

This comment has been minimized.

Copy link
Member

Sumith1896 commented Jan 22, 2016

+1 to merge

@abhinavagarwal07

This comment has been minimized.

Copy link
Contributor

abhinavagarwal07 commented Jan 22, 2016

+1

@Sumith1896

This comment has been minimized.

Copy link
Member

Sumith1896 commented Jan 23, 2016

@CodeMaxx Could you squash your commits?
We can merge it after that.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 23, 2016

@Sumith1896 Yeah I'll do it.

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:fix-euler-number branch from 2877398 to 94d699b Jan 23, 2016

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Jan 23, 2016

Looks good. Can you add a couple of tests as well?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 23, 2016

@isuruf Added the tests. I was wondering why so many mpfr functions are used in mpc files ?

@@ -53,5 +54,15 @@ TEST_CASE("precision: eval_mpfr", "[eval_mpfr]")
REQUIRE(mpfr_cmp_d(a, 0.00000000490153) == 1);
REQUIRE(mpfr_cmp_d(a, 0.00000000490154) == -1);

mpfr_init2(a, 100);

This comment has been minimized.

@isuruf

isuruf Jan 24, 2016

Member

This should be mpfr_set_prec(a, 100);. Initializing an already initialized mpfr_t may lead to memory leaks.

This comment has been minimized.

@CodeMaxx

CodeMaxx Jan 24, 2016

Contributor

Oh, should have seen that. I'll make the changes.

Found a similar problem at Line#46 .

@certik

This comment has been minimized.

Copy link
Contributor

certik commented Jan 27, 2016

That looks good to me, thanks!

certik added a commit that referenced this pull request Jan 27, 2016

Merge pull request #761 from CodeMaxx/fix-euler-number
Changed euler constant to euler number in eval_mpfr and eval_mpc

@certik certik merged commit e3a92d4 into symengine:master Jan 27, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 27, 2016

@certik Thanxx for the merge.

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