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

Trigonometric solvers #1305

Merged
merged 5 commits into from Sep 5, 2017

Conversation

Projects
None yet
4 participants
@ranjithkumar007
Contributor

ranjithkumar007 commented Jul 7, 2017

TODO :

  • Fix failing tests.
    I will add trig simplification in a different PR.
@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Jul 11, 2017

Contributor

would you mind reviewing this @srajangarg @Sumith1896 @nishnik ?

Contributor

ranjithkumar007 commented Jul 11, 2017

would you mind reviewing this @srajangarg @Sumith1896 @nishnik ?

@ranjithkumar007 ranjithkumar007 changed the title from [WIP] Trigonometric solvers to Trigonometric solvers Jul 12, 2017

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Jul 12, 2017

Contributor

One test doesn't pass because expand(I*(x + y)) doesn't simplify. (unrelated to this PR)
I will try to fix it sometime later.

Contributor

ranjithkumar007 commented Jul 12, 2017

One test doesn't pass because expand(I*(x + y)) doesn't simplify. (unrelated to this PR)
I will try to fix it sometime later.

@isuruf

This comment has been minimized.

Show comment
Hide comment
@isuruf

isuruf Jul 12, 2017

Member

Can you send smaller PRs please? This PR has lots of independent stuff.

Member

isuruf commented Jul 12, 2017

Can you send smaller PRs please? This PR has lots of independent stuff.

Show outdated Hide outdated symengine/functions.cpp
map_basic_basic mp_args;
for (const auto &a : self->get_args()) {
mp_args[a] = expand_as_exp(a);
}

This comment has been minimized.

@isuruf

isuruf Jul 12, 2017

Member

This is really inefficient. I'd rewrite this after looking at SubsVisitor.

@isuruf

isuruf Jul 12, 2017

Member

This is really inefficient. I'd rewrite this after looking at SubsVisitor.

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Jul 13, 2017

Contributor
template <typename Func>
RCP<const Basic> bottom_up(const RCP<const Basic> &barg, const Func &f)
{
    if (is_a<Add>(*barg) or is_a<Mul>(*barg)) {
        vec_basic newargs;
        for (const auto &a : barg->get_args()) {
            newargs.push_back(bottom_up(a, f));
        }
        if (is_a<Add>(*barg))
            return f(add(newargs));
        return f(mul(newargs));
    }
    if (is_a<Pow>(*barg)) {
        auto &dfunc = down_cast<const Pow &>(*barg);
        auto farg1 = dfunc.get_base(), farg2 = dfunc.get_exp();
        auto newarg1 = bottom_up(farg1, f), newarg2 = bottom_up(farg2, f);
        auto nbarg = barg;
        if (farg1 != newarg1 or farg2 != newarg2)
            nbarg = pow(newarg1, newarg2);
        return f(nbarg);
    }
    if (is_a_sub<OneArgFunction>(*barg)) {
        const OneArgFunction &dfunc = down_cast<const OneArgFunction &>(*barg);
        auto farg = dfunc.get_arg();
        auto newarg = bottom_up(farg, f);
        auto nbarg = barg;
        if (not eq(*newarg, *farg))
            nbarg = dfunc.create(newarg);
        return f(nbarg);
    }
    if (is_a_sub<TwoArgFunction>(*barg)) {
        const TwoArgFunction &dfunc = down_cast<const TwoArgFunction &>(*barg);
        auto farg1 = dfunc.get_arg1(), farg2 = dfunc.get_arg2();
        auto newarg1 = bottom_up(farg1, f), newarg2 = bottom_up(farg2, f);
        auto nbarg = barg;
        if (farg1 != newarg1 or farg2 != newarg2)
            nbarg = dfunc.create(newarg1, newarg2);
        return f(nbarg);
    }
    if (is_a_sub<MultiArgFunction>(*barg)) {
        const MultiArgFunction &dfunc
            = down_cast<const MultiArgFunction &>(*barg);
        auto fargs = dfunc.get_args();
        vec_basic newargs;
        for (const auto &a : fargs) {
            newargs.push_back(bottom_up(a, f));
        }
        auto nbarg = dfunc.create(newargs);
        return f(nbarg);
    }
    return barg;
}

RCP<const Basic> expand_as_exp(const RCP<const Basic> &self)
{
    // map_basic_basic mp_args;
    // for (const auto &a : self->get_args()) {
    //     mp_args[a] = expand_as_exp(a);
    // }

    static auto f = [](const RCP<const Basic> &a) -> RCP<const Basic> {
        if (is_a<Sin>(*a) or is_a<Cos>(*a) or is_a<Tan>(*a)
            or is_a<Cot>(*a) or is_a<Sec>(*a) or is_a<Csc>(*a)
            or is_a<Sinh>(*a) or is_a<Cosh>(*a) or is_a<Tanh>(*a)
            or is_a<Coth>(*a) or is_a<Sech>(*a)
            or is_a<Csch>(*a)) // expandables
        {
            const OneArgFunction &func = down_cast<const OneArgFunction &>(*a);
            return func.expand_as_exp();
        }
        return a;
    };
    return bottom_up(self, f);
    // return self->subs(mp_args);
}

@isuruf Is this the efficient way?

@ranjithkumar007

ranjithkumar007 Jul 13, 2017

Contributor
template <typename Func>
RCP<const Basic> bottom_up(const RCP<const Basic> &barg, const Func &f)
{
    if (is_a<Add>(*barg) or is_a<Mul>(*barg)) {
        vec_basic newargs;
        for (const auto &a : barg->get_args()) {
            newargs.push_back(bottom_up(a, f));
        }
        if (is_a<Add>(*barg))
            return f(add(newargs));
        return f(mul(newargs));
    }
    if (is_a<Pow>(*barg)) {
        auto &dfunc = down_cast<const Pow &>(*barg);
        auto farg1 = dfunc.get_base(), farg2 = dfunc.get_exp();
        auto newarg1 = bottom_up(farg1, f), newarg2 = bottom_up(farg2, f);
        auto nbarg = barg;
        if (farg1 != newarg1 or farg2 != newarg2)
            nbarg = pow(newarg1, newarg2);
        return f(nbarg);
    }
    if (is_a_sub<OneArgFunction>(*barg)) {
        const OneArgFunction &dfunc = down_cast<const OneArgFunction &>(*barg);
        auto farg = dfunc.get_arg();
        auto newarg = bottom_up(farg, f);
        auto nbarg = barg;
        if (not eq(*newarg, *farg))
            nbarg = dfunc.create(newarg);
        return f(nbarg);
    }
    if (is_a_sub<TwoArgFunction>(*barg)) {
        const TwoArgFunction &dfunc = down_cast<const TwoArgFunction &>(*barg);
        auto farg1 = dfunc.get_arg1(), farg2 = dfunc.get_arg2();
        auto newarg1 = bottom_up(farg1, f), newarg2 = bottom_up(farg2, f);
        auto nbarg = barg;
        if (farg1 != newarg1 or farg2 != newarg2)
            nbarg = dfunc.create(newarg1, newarg2);
        return f(nbarg);
    }
    if (is_a_sub<MultiArgFunction>(*barg)) {
        const MultiArgFunction &dfunc
            = down_cast<const MultiArgFunction &>(*barg);
        auto fargs = dfunc.get_args();
        vec_basic newargs;
        for (const auto &a : fargs) {
            newargs.push_back(bottom_up(a, f));
        }
        auto nbarg = dfunc.create(newargs);
        return f(nbarg);
    }
    return barg;
}

RCP<const Basic> expand_as_exp(const RCP<const Basic> &self)
{
    // map_basic_basic mp_args;
    // for (const auto &a : self->get_args()) {
    //     mp_args[a] = expand_as_exp(a);
    // }

    static auto f = [](const RCP<const Basic> &a) -> RCP<const Basic> {
        if (is_a<Sin>(*a) or is_a<Cos>(*a) or is_a<Tan>(*a)
            or is_a<Cot>(*a) or is_a<Sec>(*a) or is_a<Csc>(*a)
            or is_a<Sinh>(*a) or is_a<Cosh>(*a) or is_a<Tanh>(*a)
            or is_a<Coth>(*a) or is_a<Sech>(*a)
            or is_a<Csch>(*a)) // expandables
        {
            const OneArgFunction &func = down_cast<const OneArgFunction &>(*a);
            return func.expand_as_exp();
        }
        return a;
    };
    return bottom_up(self, f);
    // return self->subs(mp_args);
}

@isuruf Is this the efficient way?

This comment has been minimized.

@isuruf

isuruf Jul 13, 2017

Member

Change bottom_up to a Visitor class and extend that for expand_as_exp by overriding the void bvisit(const Sin &) method and other methods.

@isuruf

isuruf Jul 13, 2017

Member

Change bottom_up to a Visitor class and extend that for expand_as_exp by overriding the void bvisit(const Sin &) method and other methods.

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Jul 13, 2017

Contributor

Thanks :)

@ranjithkumar007

ranjithkumar007 Jul 13, 2017

Contributor

Thanks :)

Show outdated Hide outdated symengine/real_imag.cpp
}
}
void bvisit(const Complex &x)

This comment has been minimized.

@isuruf

isuruf Jul 12, 2017

Member

This should be ComplexBase

@isuruf

isuruf Jul 12, 2017

Member

This should be ComplexBase

This comment has been minimized.

@isuruf

isuruf Jul 12, 2017

Member

Also need to handle zoo

@isuruf

isuruf Jul 12, 2017

Member

Also need to handle zoo

Show outdated Hide outdated symengine/real_imag.cpp
@@ -0,0 +1,133 @@
#include <symengine/visitor.h>

This comment has been minimized.

@isuruf

isuruf Jul 12, 2017

Member

Rename this file to as_real_imag.cpp

@isuruf

isuruf Jul 12, 2017

Member

Rename this file to as_real_imag.cpp

Show outdated Hide outdated symengine/real_imag.cpp
for (const auto &arg : x.get_args()) {
as_real_imag(arg, outArg(re_), outArg(im_));
re = add(re, re_);

This comment has been minimized.

@isuruf

isuruf Jul 12, 2017

Member

This is inefficient. Use Add::from_dict and helpers there.

@isuruf

isuruf Jul 12, 2017

Member

This is inefficient. Use Add::from_dict and helpers there.

Show outdated Hide outdated symengine/real_imag.cpp
is_imag = !is_imag;
coef = mul(coef, im_);
} else {
rest = mul(rest, arg);

This comment has been minimized.

@isuruf

isuruf Jul 12, 2017

Member

Why?

@isuruf
Show outdated Hide outdated symengine/real_imag.cpp
void bvisit(const Basic &x)
{
*real_ = x.rcp_from_this();

This comment has been minimized.

@isuruf

isuruf Jul 12, 2017

Member

What about trig functions?

@isuruf

isuruf Jul 12, 2017

Member

What about trig functions?

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Jul 13, 2017

Contributor

Okay! I will make smaller PRs.

Update :

Done with #1309 , #1310 , #1314.
Left is this PR itself.

Contributor

ranjithkumar007 commented Jul 13, 2017

Okay! I will make smaller PRs.

Update :

Done with #1309 , #1310 , #1314.
Left is this PR itself.

@ranjithkumar007 ranjithkumar007 referenced this pull request Jul 15, 2017

Merged

expand as exp #1309

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Jul 29, 2017

Contributor

@srajangarg, can you please review this?

Contributor

ranjithkumar007 commented Jul 29, 2017

@srajangarg, can you please review this?

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Jul 29, 2017

Contributor

Any idea on why test cases are failing on MSVC15 compiler here?

Contributor

ranjithkumar007 commented Jul 29, 2017

Any idea on why test cases are failing on MSVC15 compiler here?

@srajangarg

This comment has been minimized.

Show comment
Hide comment
@srajangarg

srajangarg Jul 30, 2017

Contributor

Can you pinpoint which tests fail?

Contributor

srajangarg commented Jul 30, 2017

Can you pinpoint which tests fail?

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Jul 30, 2017

Contributor
C:\projects\symengine\symengine\tests\basic\test_solve.cpp(388): FAILED:
  REQUIRE( ! is_a_LinearArgTrigEquation(*add(tan(x), x), *x) )
with expansion:
  false
Contributor

ranjithkumar007 commented Jul 30, 2017

C:\projects\symengine\symengine\tests\basic\test_solve.cpp(388): FAILED:
  REQUIRE( ! is_a_LinearArgTrigEquation(*add(tan(x), x), *x) )
with expansion:
  false
@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 2, 2017

Contributor

tests passed on MSVC15.
ping @srajangarg

Contributor

ranjithkumar007 commented Aug 2, 2017

tests passed on MSVC15.
ping @srajangarg

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 3, 2017

Contributor

https://travis-ci.org/symengine/symengine/jobs/259971628#L9153 shows some weird error message.
I think that error is not related to the PR.
ping @isuruf

Contributor

ranjithkumar007 commented Aug 3, 2017

https://travis-ci.org/symengine/symengine/jobs/259971628#L9153 shows some weird error message.
I think that error is not related to the PR.
ping @isuruf

@srajangarg

This comment has been minimized.

Show comment
Hide comment
@srajangarg

srajangarg Aug 3, 2017

Contributor

@isuruf can you review too?

Contributor

srajangarg commented Aug 3, 2017

@isuruf can you review too?

@srajangarg

This comment has been minimized.

Show comment
Hide comment
@srajangarg

srajangarg Aug 5, 2017

Contributor

g++-4.8: internal compiler error: Killed (program cc1plus) @isuruf compiler bug?

Contributor

srajangarg commented Aug 5, 2017

g++-4.8: internal compiler error: Killed (program cc1plus) @isuruf compiler bug?

@isuruf

This comment has been minimized.

Show comment
Hide comment
@isuruf

isuruf Aug 5, 2017

Member

Travis-ci might have killed the compiler if it was taking too much memory.

Member

isuruf commented Aug 5, 2017

Travis-ci might have killed the compiler if it was taking too much memory.

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 5, 2017

Contributor

@isuruf, how to fix this memory issue?

Contributor

ranjithkumar007 commented Aug 5, 2017

@isuruf, how to fix this memory issue?

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 6, 2017

Contributor

restarting this build on my fork ended up with a different error
mktemp: failed to create file via template ‘/tmp/codecov.adjustments.XXXXXX’: No space left on device .
Is this a problem with travis?
ping @srajangarg @isuruf

Contributor

ranjithkumar007 commented Aug 6, 2017

restarting this build on my fork ended up with a different error
mktemp: failed to create file via template ‘/tmp/codecov.adjustments.XXXXXX’: No space left on device .
Is this a problem with travis?
ping @srajangarg @isuruf

@isuruf

This comment has been minimized.

Show comment
Hide comment
@isuruf

isuruf Aug 7, 2017

Member

Can you try removing Piranha from the test?

Member

isuruf commented Aug 7, 2017

Can you try removing Piranha from the test?

Show outdated Hide outdated symengine/subs.h
if (subs_dict_.size() == 1 and is_a<Pow>(*((*subs_dict_.begin()).first))
and not is_a<Add>(
*down_cast<const Pow &>(*(*subs_dict_.begin()).first)
.get_exp())) {

This comment has been minimized.

@isuruf

isuruf Aug 9, 2017

Member

At the moment, subs in symengine is like xreplace in sympy and doesn't do these kind of simplifications. If you want to do this, I would rename the existing method to xreplace (XReplaceVisitor) and then use subs (SubsVisitor inheriting from XReplaceVisitor) for the new implementation.

@isuruf

isuruf Aug 9, 2017

Member

At the moment, subs in symengine is like xreplace in sympy and doesn't do these kind of simplifications. If you want to do this, I would rename the existing method to xreplace (XReplaceVisitor) and then use subs (SubsVisitor inheriting from XReplaceVisitor) for the new implementation.

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Aug 9, 2017

Contributor

Never noticed this before.
Thanks. will update this asap.

@ranjithkumar007

ranjithkumar007 Aug 9, 2017

Contributor

Never noticed this before.
Thanks. will update this asap.

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Aug 9, 2017

Contributor

what about msubs? Should that be replaced with mxreplace?

@ranjithkumar007

ranjithkumar007 Aug 9, 2017

Contributor

what about msubs? Should that be replaced with mxreplace?

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Aug 9, 2017

Contributor

Also, I think we need to add a class for XReplace similar to Subs?

@ranjithkumar007

ranjithkumar007 Aug 9, 2017

Contributor

Also, I think we need to add a class for XReplace similar to Subs?

This comment has been minimized.

@isuruf

isuruf Aug 10, 2017

Member

msubsvisitor should derive from xreplacevisitor.

Also, I think we need to add a class for XReplace similar to Subs? No need. Subs is enough.

@isuruf

isuruf Aug 10, 2017

Member

msubsvisitor should derive from xreplacevisitor.

Also, I think we need to add a class for XReplace similar to Subs? No need. Subs is enough.

This comment has been minimized.

@isuruf

isuruf Aug 10, 2017

Member

Also, a separate PR would be great for this.

@isuruf

isuruf Aug 10, 2017

Member

Also, a separate PR would be great for this.

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 15, 2017

Contributor

@srajangarg, I tried moving piranha to a different test here. This landed in a different issue

CMake Error at symengine/cmake_install.cmake:36 (file):
  file INSTALL cannot copy file
  "/home/travis/build/symengine/symengine/build/symengine/libsymengine.a" to
  "/home/travis/our_usr/lib/libsymengine.a".
Call Stack (most recent call first):
  cmake_install.cmake:63 (include)
Contributor

ranjithkumar007 commented Aug 15, 2017

@srajangarg, I tried moving piranha to a different test here. This landed in a different issue

CMake Error at symengine/cmake_install.cmake:36 (file):
  file INSTALL cannot copy file
  "/home/travis/build/symengine/symengine/build/symengine/libsymengine.a" to
  "/home/travis/our_usr/lib/libsymengine.a".
Call Stack (most recent call first):
  cmake_install.cmake:63 (include)
@isuruf

This landed in a different issue

It's the same issue of not enough space. Piranha with coverage runs produce lots of coverage info

Show outdated Hide outdated .travis.yml
packages:
- binutils-dev
- g++-4.7
- env: BUILD_TYPE="Debug" WITH_BFD="yes" WITH_PIRANHA="yes" WITH_COVERAGE="yes" MAKEFLAGS="-j2"

This comment has been minimized.

@isuruf

isuruf Aug 17, 2017

Member

Problem was with piranha and coverage. This changes nothing

@isuruf

isuruf Aug 17, 2017

Member

Problem was with piranha and coverage. This changes nothing

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Aug 17, 2017

Contributor

Should I disable coverage here?

@ranjithkumar007

ranjithkumar007 Aug 17, 2017

Contributor

Should I disable coverage here?

This comment has been minimized.

@isuruf

isuruf Aug 17, 2017

Member

Yes, also don't add a new test. add WITH_PIRANHA=yes to an existing test like https://github.com/symengine/symengine/blob/master/.travis.yml#L86

@isuruf

isuruf Aug 17, 2017

Member

Yes, also don't add a new test. add WITH_PIRANHA=yes to an existing test like https://github.com/symengine/symengine/blob/master/.travis.yml#L86

Show outdated Hide outdated bin/install_travis.sh
@@ -90,7 +90,7 @@ if [[ "${WITH_MPC}" == "yes" ]]; then
fi
if [[ "${WITH_FLINT}" == "yes" ]] && [[ "${WITH_FLINT_DEV}" != "yes" ]]; then
conda_pkgs="$conda_pkgs libflint=2.5.2"
conda_pkgs="$conda_pkgs libflint=2.5.2 cmake=3.6.2"

This comment has been minimized.

@isuruf

isuruf Aug 17, 2017

Member

Why?

@isuruf
@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 18, 2017

Contributor

Finally Travis Passes.
ping @isuruf

Contributor

ranjithkumar007 commented Aug 18, 2017

Finally Travis Passes.
ping @isuruf

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 21, 2017

Contributor

@nishnik can you have a look?

Contributor

ranjithkumar007 commented Aug 21, 2017

@nishnik can you have a look?

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Aug 31, 2017

Contributor

@isuruf , @nishnik can you please have a look?

Contributor

ranjithkumar007 commented Aug 31, 2017

@isuruf , @nishnik can you please have a look?

Show outdated Hide outdated symengine/visitor.cpp
@@ -175,4 +177,195 @@ void TransformVisitor::bvisit(const MultiArgFunction &x)
auto nbarg = x.create(newargs);
result_ = nbarg;
}
class IsALinearArgTrigVisitor : public BaseVisitor<IsALinearArgTrigVisitor>

This comment has been minimized.

@isuruf

isuruf Aug 31, 2017

Member

Use StopVisitor and preorder_traversal_stop here.

@isuruf

isuruf Aug 31, 2017

Member

Use StopVisitor and preorder_traversal_stop here.

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Sep 1, 2017

Contributor

@isuruf , I need to skip calling visit on arguments of Trig functions like
sin(add(x, y)) , Once I see sin with arguments linear in x, I should skip the preorder_traversal in only that subtree and continue to the next subtree. How can I do this?

@ranjithkumar007

ranjithkumar007 Sep 1, 2017

Contributor

@isuruf , I need to skip calling visit on arguments of Trig functions like
sin(add(x, y)) , Once I see sin with arguments linear in x, I should skip the preorder_traversal in only that subtree and continue to the next subtree. How can I do this?

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Sep 1, 2017

Contributor

I introduced a preorder_traversal_local_stop which can skip traversing the subtree generated by its arguments. Please let me know if that's Ok.

@ranjithkumar007

ranjithkumar007 Sep 1, 2017

Contributor

I introduced a preorder_traversal_local_stop which can skip traversing the subtree generated by its arguments. Please let me know if that's Ok.

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007
Contributor

ranjithkumar007 commented Sep 2, 2017

@certik

certik approved these changes Sep 2, 2017

That looks good to me. But I would like @isuruf to also approve it.

Show outdated Hide outdated symengine/visitor.cpp
}
}
class IsALinearArgTrigVisitor

This comment has been minimized.

@isuruf

isuruf Sep 2, 2017

Member

Can you move this to solve.cpp

@isuruf

isuruf Sep 2, 2017

Member

Can you move this to solve.cpp

This comment has been minimized.

@ranjithkumar007

ranjithkumar007 Sep 2, 2017

Contributor

cool.
IsALinearArgTrigVisitor might not be useful in general except for trig-solvers, Is this the reason for moving?

@ranjithkumar007

ranjithkumar007 Sep 2, 2017

Contributor

cool.
IsALinearArgTrigVisitor might not be useful in general except for trig-solvers, Is this the reason for moving?

This comment has been minimized.

@isuruf

isuruf Sep 2, 2017

Member

Yes, seems like it's needed for solvers.cpp only

@isuruf

isuruf Sep 2, 2017

Member

Yes, seems like it's needed for solvers.cpp only

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Sep 2, 2017

Contributor

updated @isuruf .

Contributor

ranjithkumar007 commented Sep 2, 2017

updated @isuruf .

Show outdated Hide outdated symengine/solve.h
@@ -46,6 +50,13 @@ RCP<const Set> solve_poly_quartic(const vec_basic &coeffs,
const RCP<const Set> &domain
= universalset());
bool is_a_LinearArgTrigEquation(const Basic &b, const Symbol &x);
RCP<const Set> invertComplex(const RCP<const Basic> &fX,

This comment has been minimized.

@isuruf

isuruf Sep 2, 2017

Member

Can you add a comments here about what this method does? It'd be great to have it for all the methods.

@isuruf

isuruf Sep 2, 2017

Member

Can you add a comments here about what this method does? It'd be great to have it for all the methods.

@ranjithkumar007

This comment has been minimized.

Show comment
Hide comment
@ranjithkumar007

ranjithkumar007 Sep 3, 2017

Contributor

@isuruf ping.

Contributor

ranjithkumar007 commented Sep 3, 2017

@isuruf ping.

Show outdated Hide outdated symengine/solve.h
// returns Inverse of a complex equation `fX = gY` wrt symbol `sym`.
// Dummy `nD` is used as the symbol for `ImageSet` while returning the solution
// set.

This comment has been minimized.

@isuruf

isuruf Sep 3, 2017

Member

I still don't understand what this does.

@isuruf

isuruf Sep 3, 2017

Member

I still don't understand what this does.

@certik

This comment has been minimized.

Show comment
Hide comment
@certik

certik Sep 3, 2017

Contributor

@ranjithkumar007 would you mind please rebasing this on top of the latest master and answering @isuruf's comment?

Contributor

certik commented Sep 3, 2017

@ranjithkumar007 would you mind please rebasing this on top of the latest master and answering @isuruf's comment?

@certik

This comment has been minimized.

Show comment
Hide comment
@certik

certik Sep 4, 2017

Contributor
Contributor

certik commented Sep 4, 2017

@ranjithkumar007 ping.

@certik certik merged commit 55a84e9 into symengine:master Sep 5, 2017

3 checks passed

codecov/project 83.35% (+2.28%) compared to 0b325d1
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@certik

This comment has been minimized.

Show comment
Hide comment
@certik

certik Sep 5, 2017

Contributor

That looks good, thanks!

Contributor

certik commented Sep 5, 2017

That looks good, thanks!

@ranjithkumar007 ranjithkumar007 deleted the ranjithkumar007:trigsolve branch Sep 5, 2017

isuruf pushed a commit to isuruf/symengine that referenced this pull request Aug 4, 2018

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