Automatic Evaluation of pow(RealDouble, Constant) #1177

ShikharJ · 2017-01-19T11:45:02Z

@isuruf I'am not confident that this is the expected implementation. Please review and suggest improvements.
Fixes #1174

jppelteret · 2017-01-19T12:08:23Z

symengine/pow.cpp

@@ -161,6 +163,14 @@ RCP<const Basic> pow(const RCP<const Basic> &a, const RCP<const Basic> &b)
                .pow(*rcp_static_cast<const Number>(b));
        }
    }
+    if (is_a<RealDouble>(*a) and is_a<Constant>(*b)) {


Surely this should be a Number instead of a RealDouble? This would cater for the case where a is complex, derives from NumberWrapper etc. If so, then I'm guessing that one should likely call a.pow(eval_double(*b)) or similar.

@jppelteret eval_double(*b) returns a plain double, which I think is not overloaded in the pow functions of classNumber or Complex.

Ah, fair enough. Alternatively one could then do a.pow(real_double(*b)).

jppelteret · 2017-01-19T12:08:42Z

symengine/pow.cpp

+        return number(
+            std::pow(down_cast<const RealDouble &>(*a).i, eval_double(*b)));
+    }
+    if (is_a<Constant>(*a) and is_a<RealDouble>(*b)) {


jppelteret · 2017-01-19T12:12:38Z

@ShikharJ By the way, if you write fixes #1174 in your initial commit message then the corresponding issue is automatically closed when the PR is merged 😀

isuruf · 2017-01-19T12:21:29Z

@ShikharJ, this should be handled in a more generic way. If you don't mind, I'll push some changes to this PR.

ShikharJ · 2017-01-19T12:24:41Z

@isuruf Sure.

jppelteret · 2017-01-19T12:31:45Z

Thanks again to you both for working on this issue so quickly!

isuruf · 2017-01-19T15:56:49Z

symengine/real_mpfr.cpp


 #ifdef HAVE_SYMENGINE_MPFR

+using SymEngine::eval_mpfr;
+


I don't think you need the above 2 lines

…ctly

Here Number should be an inexact number. Also added pow(Number, Constant)

ShikharJ · 2017-01-21T17:02:10Z

@isuruf @jppelteret The code coverage is passable as of now. Can you please take a glance and through and suggest improvements if necessary?

isuruf · 2017-01-21T17:08:03Z

symengine/tests/eval/test_eval_double.cpp

@@ -107,6 +107,8 @@ TEST_CASE("eval_double: eval_double", "[eval_double]")
        {SymEngine::atan2(r1, neg(r2)), 2.08867384922582},
        {mul(pi, mul(E, EulerGamma)), 4.92926836742289},
        {pow(mul(EulerGamma, r4), integer(8)), 4813.54354505117582},
+        {pow(E, real_double(0.2)), 1.22140275816017},


This test checks that whatever the result of pow(E, real_double(0.2)) can be evaluated. What this PR does is evaluating pow(E, real_double(0.2)) automatically without the need of evaluation using eval_double and the like. Can you add a test to check that pow(E, real_double(0.2)) is a RealDouble and that it's value is close to 1.22140275816017

@isuruf In which test-file would that test be the most appropiate?

See this test, https://github.com/symengine/symengine/blob/master/symengine/tests/basic/test_arit.cpp#L761-L769
You can have a new test over there.

Thanks very much for including some tests! If its appropriate, may you please also check that this works if the base or exponent is a Symbol?

@jppelteret, what do you mean by that? E**2.0 will get evaluated while x**2.0 will not be.

@isuruf I was think of the case E**x and x**E and subsequent substitution. But as I said, if I've misunderstood what's being tested then my comment should of course be ignored!

Right. That's a good test.
@ShikharJ, can you add that as well?
exp(x)->subs({{x, 1.0}}) is a RealDouble and the value is close to 2.71828182845905

jppelteret · 2017-01-21T17:23:21Z

symengine/pow.cpp

-                .pow(*rcp_static_cast<const Number>(b));
+        } else if (is_a<Constant>(*a)) {
+            RCP<const Number> p = rcp_static_cast<const Number>(b);
+            if (not p->is_exact()) {


What happens if this condition is not met?

@jppelteret The function will return make_rcp<const Pow>(a, b);. Basically, it will be printed as it is, no numeric evaluation would be done. not p->is_exact() statement holds true for RealDouble and similar classes, as a double is never exact. @isuruf Please correct me if I am mistaken.

Yes, E**2 is not evaluated automatically, but E**2.0 is evaluated automatically in the same sense that sin(2) is not evaluated automatically, but sin(2.0) is.

Oh yes, you must be right. I didn't notice the default condition at the end of the function :-) Thanks!

jppelteret · 2017-01-21T17:23:43Z

symengine/pow.cpp

+    if (is_a<Constant>(*b) and is_a_Number(*a)) {
+        // Handle 2.0**E
+        RCP<const Number> p = rcp_static_cast<const Number>(a);
+        if (not p->is_exact()) {


And ditto for this condition...

ShikharJ · 2017-01-22T07:49:27Z

ping @isuruf @jppelteret

jppelteret · 2017-01-22T08:17:49Z

Awesome. Looks good to me!

Automatic Evaluation of pow(RealDouble, Constant)

jppelteret reviewed Jan 19, 2017

View reviewed changes

ShikharJ force-pushed the Issue1174 branch from 9f832ec to 08a7146 Compare January 19, 2017 15:31

isuruf reviewed Jan 19, 2017

View reviewed changes

symengine/real_mpfr.cpp

#ifdef HAVE_SYMENGINE_MPFR

using SymEngine::eval_mpfr;

Copy link

Member

isuruf Jan 19, 2017 •

edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the above 2 lines

isuruf force-pushed the Issue1174 branch from 08a7146 to ceed155 Compare January 19, 2017 16:15

ShikharJ and others added 8 commits January 21, 2017 15:28

Evaluate pow(Constant, RealDouble) and pow(RealDouble, Constant) dire…

43e160e

…ctly

Refactor pow method

041e4c1

Add automatic evaluation for pow(Constant, Number)

4fd66a4

Here Number should be an inexact number. Also added pow(Number, Constant)

Simplify a line in pow

d046a79

Fix errors in pow(Constant, Number)

2f602ce

Remove unused headers

52874e9

Include missing header

74706f7

Fix typo in EvaluateMPC

c4d2322

ShikharJ force-pushed the Issue1174 branch 2 times, most recently from 00b226e to f68320d Compare January 21, 2017 16:08

isuruf reviewed Jan 21, 2017

View reviewed changes

jppelteret reviewed Jan 21, 2017

View reviewed changes

ShikharJ force-pushed the Issue1174 branch 2 times, most recently from 95108bb to d7b5258 Compare January 21, 2017 17:51

Tests to increase coverage

6cd7384

ShikharJ force-pushed the Issue1174 branch from d7b5258 to 6cd7384 Compare January 21, 2017 18:15

isuruf merged commit c1cd1ad into symengine:master Jan 22, 2017

ShikharJ deleted the Issue1174 branch January 22, 2017 16:48

ranjithkumar007 pushed a commit to ranjithkumar007/symengine that referenced this pull request Jan 31, 2017

Merge pull request symengine#1177 from ShikharJ/Issue1174

4c81ffe

Automatic Evaluation of pow(RealDouble, Constant)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic Evaluation of pow(RealDouble, Constant) #1177

Automatic Evaluation of pow(RealDouble, Constant) #1177

ShikharJ commented Jan 19, 2017 •

edited by isuruf

jppelteret Jan 19, 2017 •

edited

ShikharJ Jan 19, 2017

jppelteret Jan 19, 2017 •

edited

jppelteret Jan 19, 2017

jppelteret commented Jan 19, 2017

isuruf commented Jan 19, 2017

ShikharJ commented Jan 19, 2017

jppelteret commented Jan 19, 2017

isuruf Jan 19, 2017 •

edited

ShikharJ commented Jan 21, 2017

isuruf Jan 21, 2017

ShikharJ Jan 21, 2017

isuruf Jan 21, 2017

jppelteret Jan 21, 2017

isuruf Jan 21, 2017

jppelteret Jan 21, 2017

isuruf Jan 21, 2017

jppelteret Jan 21, 2017

ShikharJ Jan 21, 2017

isuruf Jan 21, 2017

jppelteret Jan 21, 2017

jppelteret Jan 21, 2017

ShikharJ commented Jan 22, 2017

jppelteret commented Jan 22, 2017

Automatic Evaluation of pow(RealDouble, Constant) #1177

Automatic Evaluation of pow(RealDouble, Constant) #1177

Conversation

ShikharJ commented Jan 19, 2017 • edited by isuruf

jppelteret Jan 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jppelteret Jan 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jppelteret commented Jan 19, 2017

isuruf commented Jan 19, 2017

ShikharJ commented Jan 19, 2017

jppelteret commented Jan 19, 2017

isuruf Jan 19, 2017 • edited

Choose a reason for hiding this comment

ShikharJ commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShikharJ commented Jan 22, 2017

jppelteret commented Jan 22, 2017

ShikharJ commented Jan 19, 2017 •

edited by isuruf

jppelteret Jan 19, 2017 •

edited

jppelteret Jan 19, 2017 •

edited

isuruf Jan 19, 2017 •

edited