Simplify Complex Arguments to Exp #1332

ShikharJ · 2017-09-27T08:13:27Z

Relevant: #1330

ShikharJ · 2017-09-27T08:16:44Z

Ping @certik.

ShikharJ · 2017-09-27T13:57:51Z

@isuruf Can you review as well?

certik

This seems like the right fix. I'll wait for @isuruf to also review it.

certik · 2017-09-27T15:08:00Z

symengine/pow.cpp

+                for (const auto &p : dict) {
+                    if (eq(*p.first, *pi) and eq(*p.second, *one)) {
+                        return add(cos(mul(mul(I, minus_one), b)),
+                                   mul(I, sin(mul(mul(I, minus_one), b))));


This seems correct, as you can always do this transformation. The only question remaining is whether this transformation is only applied when it can simplify the exp(), and it seems it is.

It is probably possible to optimize this return statement further, using the information we know (i.e. not calling sin/cos at all), but that can be done later.

certik · 2017-09-27T15:10:00Z

I don't understand how the code coverage could decrease with this PR. By clicking on it, it found some difference in add.cpp? That's weird.

isuruf · 2017-09-27T15:12:41Z

Can you add some examples like ?

exp(I*pi/2 + x)
exp(I*pi/10 + x)
exp(I*pi*x)/exp(x)

ShikharJ · 2017-10-02T15:46:34Z

Ping @isuruf

isuruf · 2017-10-02T15:48:54Z

symengine/tests/basic/test_arit.cpp

+    REQUIRE(eq(*r1, *r2));
+
+    r1 = exp(add(div(mul(I, pi), integer(10)), x));
+    r2 = mul(exp(x), exp(div(mul(I, pi), integer(10))));


Above 2 check the same thing. Can you comment here with the string form of r1?

isuruf · 2017-10-02T15:49:27Z

symengine/tests/basic/test_arit.cpp

+    REQUIRE(eq(*r1, *mul(I, minus_one)));
+
+    r1 = div(exp(mul(mul(I, pi), x)), exp(x));
+    r2 = mul(exp(mul(minus_one, x)), exp(mul(mul(I, pi), x)));


Can you comment the string form of r1?

isuruf · 2017-10-03T15:14:00Z

symengine/tests/basic/test_arit.cpp

+    REQUIRE(eq(*r1, *r2));
+
+    r1 = exp(add(div(mul(I, pi), integer(10)), x));
+    REQUIRE(r1->__str__() == "exp(x)*(I*sin((1/10)*pi) + cos((1/10)*pi))");


This is too complicated than it needs to be.

isuruf · 2017-10-03T15:14:50Z

symengine/tests/basic/test_arit.cpp

+    REQUIRE(eq(*r1, *mul(I, minus_one)));
+
+    r1 = div(exp(mul(mul(I, pi), x)), exp(x));
+    REQUIRE(r1->__str__() == "exp(-x + I*x*pi)");


Can you change this test to checking that r1 is a Pow and that the base and exp are E and -x + I * x * pi respectively ?

isuruf · 2017-10-03T15:50:14Z

symengine/pow.cpp

+                if ((is_a<Integer>(*mul(coef, I))
+                     or (is_a<Rational>(*mul(coef, I))
+                         and (eq(*mul(coef, I), *rational(1, 2))
+                              or eq(*mul(coef, I), *rational(-1, 2)))))) {


mul(coef, I) is calculated repeatedly. How about exp(3*pi*I/2)?

isuruf · 2017-10-03T15:51:31Z

symengine/pow.cpp

+            RCP<const Number> coef = down_cast<const Add &>(*b).get_coef();
+            RCP<const Basic> s = one;
+            for (const auto &p : dict) {
+                if (eq(*p.first, *pi) and is_a_Complex(*p.second)) {


Use is_a<Complex> to keep the consistency as above.

isuruf · 2017-10-03T15:53:41Z

symengine/pow.cpp

+                    for (const auto &p : dict) {
+                        if (eq(*p.first, *pi) and eq(*p.second, *one)) {
+                            return add(cos(mul(mul(I, minus_one), b)),
+                                       mul(I, sin(mul(mul(I, minus_one), b))));


Since we only care about multiples of I*pi/2, how about checking for the multiple, and writing the formula for the 4 cases? Will be faster.

To be clear, which four cases are we talking about?

If the arg is n*I*pi/2, whether n = 0,1,2,3 mod 4

isuruf · 2017-10-03T15:55:17Z

symengine/pow.cpp

+            RCP<const Basic> s = one;
+            for (const auto &p : dict) {
+                if (eq(*p.first, *pi) and is_a_Complex(*p.second)) {
+                    s = exp(mul(p.first, p.second));


Can you take out the logic of mul to a new function and then call it from here so that we know if s was simplified that it is a number, else we can do Add::dict_add_term(new_dict, p.second, p.first);

isuruf · 2017-10-03T15:56:49Z

@ShikharJ, optimizations I'm talking about maybe very small and won't matter for most of the cases, but when changing a very important function like pow, add, mul, I want to be extra careful not to add overhead.

certik · 2017-10-03T17:29:18Z

Yes, we have to be very careful when modifying core routines like in this PR.

ShikharJ · 2017-10-04T07:15:12Z

Ping @isuruf.

certik · 2017-10-04T15:16:06Z

symengine/pow.cpp

+            return mul(s, make_rcp<const Pow>(
+                              a, Add::from_dict(coef, std::move(new_dict))));
+        }
+    }


We need to carefully benchmark these two if branches, before and after this PR. So the first if clause would run for something like E^(a*b). The second one for E^(a+b). Correct?

If so, then we need to benchmark these two things and similar things a lot.

isuruf · 2017-10-04T15:41:26Z

symengine/pow.cpp

@@ -87,6 +87,25 @@ int Pow::compare(const Basic &o) const
        return base_cmp;
 }

+bool exp_mul_helper(const RCP<const Basic> &b)


How about this function returning -1, 0, 1,2,3 where -1 is when it is not what we want and the others n in the function beloe

isuruf · 2017-10-04T16:01:12Z

symengine/pow.cpp

+            RCP<const Number> coef = down_cast<const Add &>(*b).get_coef();
+            RCP<const Basic> s = one;
+            for (const auto &p : dict) {
+                if (eq(*p.first, *pi)


To do this, you don't need to iterate the dictionary. Just use dict.find to find the value pi.

Is there a way I can remove that particular entry of pi as well? That way, there'd be no need of iteration at all.

isuruf · 2017-10-04T16:02:19Z

symengine/pow.cpp

+            for (const auto &p : dict) {
+                if (eq(*p.first, *pi)
+                    and exp_mul_helper(mul(p.first, p.second))) {
+                    s = exp(mul(p.first, p.second));


Can a call to exp be avoided here?

At the expense of code duplication it can be. I can't seem to think of another way.

certik · 2017-11-02T23:16:54Z

What needs to be done here to finish it?

certik approved these changes Sep 27, 2017

View reviewed changes

ShikharJ force-pushed the Issue1330 branch from c779528 to 1f2db39 Compare September 27, 2017 17:45

ShikharJ requested a review from isuruf September 28, 2017 09:44

isuruf reviewed Oct 2, 2017

View reviewed changes

Fixes symengine#1330

1716ceb

ShikharJ force-pushed the Issue1330 branch from 1f2db39 to 1716ceb Compare October 2, 2017 16:02

ShikharJ requested a review from isuruf October 3, 2017 14:09

isuruf reviewed Oct 3, 2017

View reviewed changes

Minor Improvement

cc81591

ShikharJ force-pushed the Issue1330 branch from c6fea62 to cc81591 Compare October 3, 2017 18:34

certik reviewed Oct 4, 2017

View reviewed changes

isuruf reviewed Oct 4, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Complex Arguments to Exp #1332

Simplify Complex Arguments to Exp #1332

ShikharJ commented Sep 27, 2017

ShikharJ commented Sep 27, 2017

ShikharJ commented Sep 27, 2017

certik left a comment

certik Sep 27, 2017

certik commented Sep 27, 2017

isuruf commented Sep 27, 2017

ShikharJ commented Oct 2, 2017

isuruf Oct 2, 2017

isuruf Oct 2, 2017

isuruf Oct 3, 2017

isuruf Oct 3, 2017

isuruf Oct 3, 2017 •

edited

isuruf Oct 3, 2017

isuruf Oct 3, 2017

ShikharJ Oct 3, 2017

isuruf Oct 3, 2017

isuruf Oct 3, 2017

isuruf commented Oct 3, 2017

certik commented Oct 3, 2017

ShikharJ commented Oct 4, 2017

certik Oct 4, 2017

isuruf Oct 4, 2017

isuruf Oct 4, 2017

ShikharJ Oct 22, 2017

isuruf Oct 4, 2017

ShikharJ Oct 22, 2017 •

edited

certik commented Nov 2, 2017

Simplify Complex Arguments to Exp #1332

Are you sure you want to change the base?

Simplify Complex Arguments to Exp #1332

Conversation

ShikharJ commented Sep 27, 2017

ShikharJ commented Sep 27, 2017

ShikharJ commented Sep 27, 2017

certik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik commented Sep 27, 2017

isuruf commented Sep 27, 2017

ShikharJ commented Oct 2, 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

isuruf Oct 3, 2017 • edited

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

isuruf commented Oct 3, 2017

certik commented Oct 3, 2017

ShikharJ commented Oct 4, 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

ShikharJ Oct 22, 2017 • edited

Choose a reason for hiding this comment

certik commented Nov 2, 2017

isuruf Oct 3, 2017 •

edited

ShikharJ Oct 22, 2017 •

edited