-
Notifications
You must be signed in to change notification settings - Fork 269
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
Simplify Complex Arguments to Exp #1332
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,6 +164,34 @@ RCP<const Basic> pow(const RCP<const Basic> &a, const RCP<const Basic> &b) | |
RCP<const Pow> A = rcp_static_cast<const Pow>(a); | ||
return pow(A->get_base(), mul(A->get_exp(), b)); | ||
} | ||
if (eq(*a, *E)) { | ||
if (is_a<Mul>(*b) | ||
and is_a_Complex(*down_cast<const Mul &>(*b).get_coef())) { | ||
const map_basic_basic &dict = down_cast<const Mul &>(*b).get_dict(); | ||
if (dict.size() == 1) { | ||
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)))); | ||
} | ||
} | ||
} | ||
} else if (is_a<Add>(*b)) { | ||
const umap_basic_num &dict = down_cast<const Add &>(*b).get_dict(); | ||
umap_basic_num new_dict; | ||
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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
s = exp(mul(p.first, p.second)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you take out the logic of mul to a new function and then call it from here so that we know if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can a call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the expense of code duplication it can be. I can't seem to think of another way. |
||
} else { | ||
Add::dict_add_term(new_dict, p.second, p.first); | ||
} | ||
} | ||
return mul(s, make_rcp<const Pow>( | ||
a, Add::from_dict(coef, std::move(new_dict)))); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to carefully benchmark these two if branches, before and after this PR. So the first if clause would run for something like If so, then we need to benchmark these two things and similar things a lot. |
||
return make_rcp<const Pow>(a, b); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -987,6 +987,36 @@ TEST_CASE("Pow: arit", "[arit]") | |
r1 = pow(mul(sqrt(mul(y, x)), x), i2); | ||
r2 = mul(pow(x, i3), y); | ||
REQUIRE(eq(*r1, *r2)); | ||
|
||
r1 = exp(mul(I, x)); | ||
r2 = pow(E, mul(I, x)); | ||
REQUIRE(eq(*r1, *r2)); | ||
|
||
r1 = exp(mul(I, pi)); | ||
r2 = pow(E, mul(I, pi)); | ||
REQUIRE(eq(*r1, *r2)); | ||
|
||
r1 = exp(mul(I, pi)); | ||
REQUIRE(eq(*r1, *minus_one)); | ||
|
||
r1 = exp(mul(mul(I, minus_one), pi)); | ||
REQUIRE(eq(*r1, *minus_one)); | ||
|
||
r1 = exp(div(mul(I, pi), integer(2))); | ||
REQUIRE(eq(*r1, *I)); | ||
|
||
r1 = exp(mul(div(mul(I, pi), integer(2)), minus_one)); | ||
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)"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you change this test to checking that |
||
|
||
r1 = exp(add(div(mul(I, pi), integer(2)), x)); | ||
r2 = mul(I, exp(x)); | ||
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))"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is too complicated than it needs to be. |
||
} | ||
|
||
TEST_CASE("Log: arit", "[arit]") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.