modified lie_group method #17527

RituRajSingh878 · 2019-08-27T17:42:39Z

References to other Issues or PRs

Related to #17322 and #17338

Brief description of what is fixed or changed

lie_group has some bug with solution and some modification that I need for the pr #17338

Other comments

Release Notes

solvers
- removed lie_group bug

sympy-bot · 2019-08-27T17:42:40Z

✅

Hi, I am the SymPy bot (v148). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

solvers
- removed lie_group bug (#17527 by @RituRajSingh878)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
Related to #17322 and #17338

#### Brief description of what is fixed or changed

lie_group has some bug with solution and some modification that I need for the pr #17338 
#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- solvers
  - removed lie_group bug
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

codecov · 2019-08-27T19:05:14Z

Codecov Report

Merging #17527 into master will increase coverage by 0.039%.
The diff coverage is 62.886%.

@@              Coverage Diff              @@
##            master    #17527       +/-   ##
=============================================
+ Coverage   74.712%   74.752%   +0.039%     
=============================================
  Files          633       633               
  Lines       164497    164745      +248     
  Branches     38622     38700       +78     
=============================================
+ Hits        122900    123151      +251     
+ Misses       36204     36189       -15     
- Partials      5393      5405       +12

oscarbenjamin · 2019-08-28T02:00:50Z

This looks good. Can you give an example of what it fixes (and maybe add that as a test)?

RituRajSingh878 · 2019-08-28T02:59:40Z

sympy/solvers/ode.py

@@ -5788,6 +5789,8 @@ def ode_lie_group(eq, func, order, match):
            sol = solve(eq, df)
            if sol == []:
                raise NotImplementedError
+            elif len(sol)>1:
+                return [ode_lie_group(Eq(df, i), func, order, match) for i in sol]


In lie_group method, We are solving an equation in which we need h(x, y) see the last equation of lie_group method(https://docs.sympy.org/latest/modules/solvers/ode.html#lie-group).
And We are getting h(x, y) by solving the solve(eq, df) and initially if at least one solution exists it means at least one h(x, y) (h = sol[0].subs(func, y))exists and then we are trying to find the solution for only one of the h(x, y) but if more than one solution is existing then we will get different values of h(x, y) and so maybe for different values of h(x, y) we can get different solution or maybe same. So I am just trying to solve for each of the different values of h(x,y)

I think initially, the author wasn't checking for the different values of h(x, y) because of the condition of dsolve(We can't return a list of a solution). But now We can.

If one call to ode_lie_group returns a solution and another raises then this won't return anything.

okay, We can try to find the solution by using try and except

RituRajSingh878 · 2019-08-28T03:02:13Z

sympy/solvers/ode.py

@@ -5838,6 +5841,8 @@ def ode_lie_group(eq, func, order, match):
                scoord = newcoord[-1]
                try:
                    sol = solve([r - rcoord, s - scoord], x, y, dict=True)
+                    if sol == []:
+                        raise NotImplementedError


some time solve is returning [] so I am just trying to check this condition and returning NotImplementedError because we are accessing sol[0] and if sol = [] then it will return IndexError

Here We can solve the differential equation by lie_group only if at least one solution exists.

I think continue rather than raise NotImplementedError would be better.

The impact will be the same but I think raising NotImplementedError would be better because theoretically lie_group can always solve any differential equation then in this method here at this position there should always be a solution for the equation and if solve is not able to give the solution it means method is not implemented in the solve so we should return NotImplementedError.

I don't know about solve. If solve return an empty list then what it means?
If it means no solution is exiting for the given equation then maybe lie_group has some bug.

The Lie group method works by trying out different heuristics. None of them is guaranteed to work for any particular ODE so the solver just tries them one after another. That's why we have a loop over the heuristics here. We need to make sure that if a particular heuristic doesn't work (which could be because solve returns an empty list or raises NotImplementedError) then the loop goes on to the next heuristic. Only when all of the heuristics has failed should we go on to raise NotImplementedError at the end.

The solve function will return an empty list if there are no solutions to the equation(s) or if the solution is infinite or possibly if there is an infinite number of solutions e.g.:

In [9]: solve(exp(x)) Out[9]: [] In [10]: solve([x-1, x-2], x) Out[10]: []

The Lie group method works by trying out different heuristics. None of them is guaranteed to work for any particular ODE so the solver just tries them one after another. That's why we have a loop over the heuristics here. We need to make sure that if a particular heuristic doesn't work (which could be because solve returns an empty list or raises NotImplementedError) then the loop goes on to the next heuristic. Only when all of the heuristics has failed should we go on to raise NotImplementedError at the end.

By raising NotImplementedError in try it will go into except and from there continue will work and the loop will run and will try for the different heuristics. I was saying that If we will raise an error or do continue, it will have the same effect. I just wanted to know what will be the best idea to write.

I think that the "best" thing would be to factor the body of the loop out into a separate function and use return None rather than exceptions or continue.

RituRajSingh878 · 2019-08-28T03:09:28Z

This looks good. Can you give an example of what it fixes (and maybe add that as a test)?

eq = (f(x).diff(x)-f(x)) * (f(x).diff(x)+f(x))

if we will solve this equation by lie_group initially we will only get one solution

In [3]: dsolve(eq, hint='lie_group')
Out[3]: 
          -x
f(x) = C₁⋅ℯ

But now we will get a list of solution

Out[2]: 
⎡           -x             x⎤
⎣f(x) = C₁⋅ℯ  , f(x) = C₁⋅ℯ ⎦

oscarbenjamin · 2019-08-28T22:11:03Z

sympy/solvers/tests/test_ode.py

+def test_issue_17322():
+    eq = (f(x).diff(x)-f(x)) * (f(x).diff(x)+f(x))
+    sol = [Eq(f(x), C1*exp(-x)), Eq(f(x), C1*exp(x))]
+    assert str(sol) == str(dsolve(eq, hint='lie_group'))


Why are you using str here?

Sorry, I want to make it set by mistake I have made it str.

sympy/solvers/ode.py

oscarbenjamin · 2019-08-29T19:57:57Z

sympy/solvers/ode.py

+        tempsol = []  # Used by solve below
+        for heuristic in heuristics:
+            try:
+                if not inf:


I would take the body of this loop out into a separate _ode_lie_group_try_heuristic function. (and probably move both functions out of ode_lie_group to top level).

Actually what makes most sense is to take out the body of the infsim loop below. There is far too much nesting here. The code below could be simplified if it was in a function and could use return to exit.

It isn't necessary to change that though in this PR if you don't want to.

I need to complete this pr as soon as possible because my previous prs depends on this pr so I will do this work in other pr.

Okay that's fine. The only thing that needs fixing in this PR is to restore the error message.

oscarbenjamin · 2019-08-29T19:58:47Z

sympy/solvers/ode.py

+            if isinstance(sol, list):
+                desols.extend(sol)
+            else:
+                desols.append(sol)


If you didn't unpack the list above this wouldn't be necessary

RituRajSingh878 · 2019-08-29T21:49:58Z

sympy/solvers/ode.py

-    raise NotImplementedError("The given ODE " + str(eq) + " cannot be solved by"
-        + " the lie group method")
+    if desols == []:
+        raise NotImplementedError("Unable...")


I will change this in later pr.

I think that message should be restored in this PR

sorry, I mean commit

oscarbenjamin · 2019-08-29T23:00:08Z

sympy/solvers/ode.py

+        if isinstance(exprs, list):
+            rv = [_handle_Integral(expr, func, hint) for expr in exprs]
+        else:
+            rv = _handle_Integral(exprs, func, hint)


We really need to clean it up so that solvefunc always returns a list. We shouldn't get into this situation where we don't know if we have a list or not.

I need to open a new pr for this because initially dsolve is made in a way that mostly it will return a single equation solution. So almost all of the method return an equation. So I have to change this for many of the methods. For this pr We can go in this way and later I will change it.

oscarbenjamin · 2019-08-29T23:01:42Z

sympy/solvers/ode.py

+        try:
+            return _ode_lie_group_try_heuristic(Eq(df, s), heuristic, func, match, inf)
+        except ValueError:
+            continue


It's not obvious to me where this ValueError is coming from. Can we move this inside _ode_lie_group_try_heuristic and have that function return None instead of leaking ValueError?

We can't remove ValueError because it is coming from infinitesimals function.

Then we should catch it when calling infinitesimals rather than here. It's much better if we can arrange it so that we have functions that return values (e.g. None) to indicate failure rather than raising and catching generic exceptions like ValueError.

oscarbenjamin · 2019-08-29T23:06:25Z

sympy/solvers/ode.py

+            return _ode_lie_group_try_heuristic(Eq(df, s), heuristic, func, match, inf)
+        except ValueError:
+            continue
+    return []


The convention for dsolve is to raise NotImplementedError when an ODE can't be solved:

In [8]: dsolve(f(x).diff(x, 2)-exp(f(x))) --------------------------------------------------------------------------- NotImplementedError

We shouldn't return an empty list unless we know that there are no solutions. That might happen as a result of the initial conditions being inconsistent for example. I don't think we ever know from the Lie group methods that there are no solutions because there could always be more heuristics to try (I might be wrong about that though).

I don't think we ever know from the Lie group methods that there are no solutions because there could always be more heuristics to try (I might be wrong about that though).

Yes, we can't say ever that solution doesn't exist if we are not able to solve for the predefined heuristic.

Here I am returning an empty list (in _ode_lie_group function) because I am trying to find solutions for every element in sol=solve(eq, df) so we can't raise NotImplementedError without checking for every term in sol.
But in the end, when we are not getting any solutions then I am raising NotImplementedError(in ode_lie_group).
And I don't understand why you have given that example and the given example is not solvable by any method.

In [7]: classify_ode(f(x).diff(x, 2)-exp(f(x))) Out[7]: ()

RituRajSingh878 · 2019-08-30T13:17:39Z

sympy/solvers/ode.py

+    h = match['h']
+    tempsol = []
+    if not inf:
+        inf = infinitesimals(eq, hint=heuristic, func=func, order=1, match=match)


ValueError is coming from here.

When we will try to find infinitesimals and maybe for the heuristic, infinitesimals doesn't exists so it will raise ValueError.

oscarbenjamin · 2019-08-31T10:22:23Z

Looks good. Thanks

modified lie_group method

64a2730

oscarbenjamin added the solvers.dsolve label Aug 28, 2019

RituRajSingh878 commented Aug 28, 2019

View reviewed changes

updated lie_group and added test cases

85ac26f

oscarbenjamin reviewed Aug 28, 2019

View reviewed changes

sympy/solvers/ode.py Outdated Show resolved Hide resolved

modified lie_group method

fa00a5a

oscarbenjamin reviewed Aug 29, 2019

View reviewed changes

sympy/solvers/ode.py Outdated Show resolved Hide resolved

oscarbenjamin reviewed Aug 29, 2019

View reviewed changes

break ode_lie_group functions into different functions

c580aab

RituRajSingh878 commented Aug 29, 2019

View reviewed changes

oscarbenjamin reviewed Aug 29, 2019

View reviewed changes

RituRajSingh878 commented Aug 30, 2019

View reviewed changes

RituRajSingh878 added 2 commits August 30, 2019 13:25

modified lie_group method

ae1b5de

modified _ode_lie_group method

cfaddd6

oscarbenjamin merged commit e3c76d0 into sympy:master Aug 31, 2019

modified lie_group method #17527

modified lie_group method #17527

Conversation

RituRajSingh878 commented Aug 27, 2019 • edited Loading

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Aug 27, 2019 • edited Loading

codecov bot commented Aug 27, 2019 • edited Loading

Codecov Report

oscarbenjamin commented Aug 28, 2019

RituRajSingh878 Aug 28, 2019 • edited Loading

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

RituRajSingh878 Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RituRajSingh878 Aug 28, 2019 • edited Loading

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

RituRajSingh878 commented Aug 28, 2019

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

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

RituRajSingh878 Aug 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarbenjamin commented Aug 31, 2019

RituRajSingh878 commented Aug 27, 2019 •

edited

Loading

sympy-bot commented Aug 27, 2019 •

edited

Loading

codecov bot commented Aug 27, 2019 •

edited

Loading

RituRajSingh878 Aug 28, 2019 •

edited

Loading

RituRajSingh878 Aug 28, 2019 •

edited

Loading

RituRajSingh878 Aug 28, 2019 •

edited

Loading

RituRajSingh878 Aug 30, 2019 •

edited

Loading