Exact ODE solver class added #18887

tnzl · 2020-03-17T07:02:16Z

References to other Issues or PRs

Refactoring efforts in accordance with #18348.

Brief description of what is fixed or changed

Exact ODE solver has been added with conditions to convert given ODE to exact where possible.
ode_classify() in ode.py has been updated to use the FirstExact solver class.
_handle_Integral() in ode.py has been refactored to remove global y hack.

Other comments

Release Notes

solvers
- Added FirstExact solver class for the exact ordinary differential equation.
functions
- Refactored ode_classify() and _handle_Integral() in ode.py

…d _handle_Integral() functions in ode.py

…get_general_solution() of FirstExact class.

sympy-bot · 2020-03-17T07:02:18Z

✅

Hi, I am the SymPy bot (v158). 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:

functions
- Refactored ode_classify() and _handle_Integral() in ode.py (#18887 by @invalid-email-address and @tnzl)
solvers
- Added FirstExact solver class for the exact ordinary differential equation. (#18887 by @invalid-email-address and @tnzl)

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

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://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Refactoring efforts in accordance with #18348.

#### Brief description of what is fixed or changed
Exact ODE solver has been added with conditions to convert given ODE to exact where possible.
ode_classify() in ode.py has been updated to use the FirstExact solver class.
_handle_Integral() in ode.py has been refactored to remove global y hack.

#### 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
  * Added FirstExact solver class for the exact ordinary differential equation.

* functions
  * Refactored ode_classify() and _handle_Integral() in ode.py
<!-- END RELEASE NOTES -->

oscarbenjamin · 2020-03-17T10:19:02Z

sympy/solvers/ode/single.py

+        y=Dummy('y')
+
+        self.q=collect(eq,f.diff(x)).coeff(f.diff(x))
+        self.p=eq-self.q*f.diff(x)


It would be better to use the pattern matching approach like FirstPatternSolver

Thanks for the suggestion, I will change it to inherit SinglePatternODESolver.

oscarbenjamin · 2020-03-17T10:19:56Z

sympy/solvers/ode/single.py

+    def _matches(self) -> bool:
+        if self.ode_problem.order!=1:
+            self._matched=False
+            return False


It isn't necessary to set _matched in this function

Yeah! matches() sets _matched. Updating it.

oscarbenjamin · 2020-03-17T10:20:07Z

sympy/solvers/ode/single.py

+    hint="1st_exact"
+    has_integral=True
+    p=None
+    q=None


There should be spaces around =

oscarbenjamin · 2020-03-17T10:21:52Z

sympy/solvers/ode/single.py

+class FirstExact(SingleODESolver):
+    r"""Solves 1st order exact ordinary differential equations.
+    A 1st order differential equation is called exact if it is the total
+    differential of a function. That is, the differential equation:


There already is a docstring for this method in ode_1st_exact. The ode_1st_exact function should be removed but the docstring can be moved here

oscarbenjamin · 2020-03-17T10:22:24Z

sympy/solvers/ode/single.py

+        try:
+            sol=sol.doit()
+            gen_sol=Eq(sol.subs(y,f),C1)
+        except:


Bare except should not be used anywhere in the codebase

Removing try except block as doit will not be called here.

oscarbenjamin · 2020-03-17T10:22:57Z

sympy/solvers/ode/single.py

+
+        try:
+            sol=sol.doit()
+            gen_sol=Eq(sol.subs(y,f),C1)


We don't need to call doit here as it will be called by dsolve later.

Updating it. As there is no other way to pass y to _handle_Integral, global y hack will still be in use.

oscarbenjamin · 2020-03-17T10:23:31Z

sympy/solvers/ode/single.py

+        try:
+            if simplify(self.p)!=0:
+                m=self.p.subs(f,y)
+                n=self.q.subs(f,y)


There should be spaces after comma (see PEP8)

Thanks for the suggestion, I will format to PEP8 style.

oscarbenjamin · 2020-03-18T19:20:00Z

I'm not sure the global y is needed at all because we can integrate with functions as integration variables:

In [5]: Integral(f(x), (f(x), 0, 1))                                                                                              
Out[5]: 
1             
⌠             
⎮ f(x) d(f(x))
⌡             
0             

In [6]: Integral(f(x), (f(x), 0, 1)).doit()                                                                                       
Out[6]: 1/2

Maybe it's just not necessary to substitute f(x) for y in the first place. Can the hack be removed like that?

oscarbenjamin · 2020-03-18T19:26:11Z

sympy/solvers/ode/single.py

+    def _wilds(self, f, x, order):
+        P = Wild('P')
+        Q = Wild('Q', exclude=[f(x).diff(x)])
+        return P, Q


Should P also exclude f(x).diff(x)?

What happens with something like this?

1/f(x).diff(x) + f(x).diff(x)

Pattern match fails for equations which have Derivative in more than one term if P excludes f(x).diff(x), like eq=x*f(x).diff(x)+(2/f(x))*f(x).diff(x)+f(x).

Should P also exclude f(x).diff(x)?

What happens with something like this?

1/f(x).diff(x) + f(x).diff(x)

At present _matches checks equations of degree 1 only.

Maybe we should collect on derivatives here:

sympy/sympy/solvers/ode/single.py

Line 240 in 233cafb

eq = eq.collect(f(x), func = cancel)

oscarbenjamin · 2020-03-18T19:27:04Z

sympy/solvers/ode/single.py

+        if fx.diff(x) in self._wilds_match[P].atoms(Derivative):
+            eq = collect(eq,fx.diff(x))
+            pattern = self._equation(fx, x, 1)
+            self._wilds_match = eq.match(pattern)


I don't understand what this is doing but it doesn't seem right

The _wilds_match variable should be set by PatternSolver.match.

For the case where Derivative is present in more then one terms, eq needs to be preprocessed before matching. This condition is required to process and verify that P is the function of x , f(x) only and independent of any derivative.

I think that should be handled by excluding f(x).diff(x) in the pattern

The reason for not excluding f(x).diff(x) in Wild P and the need for this condition here are the same. Lets take an example of exact equation:

>>> P1 = Wild('P1',exclude=[f(x).diff(x)]) >>> Q1 = Wild('Q1',exclude=[f(x).diff(x)]) >>> >>> P2 = Wild('P2') >>> Q2 = Wild('Q2',exclude=[f(x).diff(x)]) >>> >>> exprn1= P1 + Q1*f(x).diff(x) >>> exprn2= P2 + Q2*f(x).diff(x) >>> >>> eq=x*f(x).diff(x)+(2/f(x))*f(x).diff(x)+f(x) >>> eq = eq.collect(f(x), func = cancel) >>> >>> print(eq.match(exprn1)) None >>> eq.match(exprn2) {Q2_: 2/f(x), P2_: x*Derivative(f(x), x) + f(x)}

Here, match with exprn1 returns None. Hence, excluding f(x).diff(x) in Wild P will fail the matches function for for the case where f(x).diff(x) is present in more then one terms of eq.
After we match if f(x).diff(x) is present in P, we update it in above if block in _verify function.

Every PatternODESolver will have different preprocess for matching. We need to define a _preprocess_eq function in the base class which preprocesses equation before it is matched in _matches function.

We need to collect on the derivatives of f as well:

In [7]: eq.collect(f(x).diff(x)) Out[7]: ⎛ 2 ⎞ d ⎜x + ────⎟⋅──(f(x)) + f(x) ⎝ f(x)⎠ dx In [8]: eq.collect(f(x).diff(x)).match(exprn1) Out[8]: ⎧ 2 ⎫ ⎨P₁: f(x), Q₁: x + ────⎬ ⎩ f(x)⎭

Absolutely. We need to collect on the derivatives of f, but this is not required by other solvers. Therefore, putting eq.collect(f(x).diff(x)) in _matches of SinglePatternODESolver will not be a wise option.
We need _preprocess_eq in SinglePatternODESolver:

#Subclass should define here all preprocess on the equation required before pattern matching. def _preprocess_eq(self, eq): prep_eq=eq return prep_eq

eq = _preprocess_eq(eq) will be called before eq.match(pattern) here:

sympy/sympy/solvers/ode/single.py

Line 242 in 233cafb

self._wilds_match = match = eq.match(pattern)

After _preprocess_eq is defined in SinglePatternODESolver, we would implement it in FirstExact as:

def _preprocess_eq(self, eq): prep_eq = eq.collect(f(x).diff(x)) return prep_eq

This will solve the problem of specific preprocessing required by different solvers before pattern matching

We need to minimise solver-specific logic. If collecting on f(x).diff(x) is generally useful then it should be done in the SingleODEPatternSolver class. Is there any reason not to put it there?

Normally collecting on f(x) also collects on derivatives e.g.:

In [60]: eq = f(x).diff(x) + x*f(x).diff(x) + f(x) + x*f(x) In [61]: eq Out[61]: d d x⋅f(x) + x⋅──(f(x)) + f(x) + ──(f(x)) dx dx In [62]: eq.collect(f(x)) Out[62]: d (x + 1)⋅f(x) + (x + 1)⋅──(f(x)) dx

The reason that doesn't work in your example is because you have mixed products involving both f(x) and f(x).diff(x):

In [63]: eq = f(x).diff(x) + (x/f(x))*f(x).diff(x) + f(x) + x*f(x) In [64]: eq Out[64]: d x⋅──(f(x)) dx d x⋅f(x) + ────────── + f(x) + ──(f(x)) f(x) dx In [65]: eq.collect(f(x)) Out[65]: d x⋅──(f(x)) dx d ────────── + (x + 1)⋅f(x) + ──(f(x)) f(x) dx In [66]: eq.collect(f(x)).collect(f(x).diff(x)) Out[66]: ⎛ x ⎞ d (x + 1)⋅f(x) + ⎜──── + 1⎟⋅──(f(x)) ⎝f(x) ⎠ dx

I think that the right way to do this is like this in the first place:

In [68]: eq.collect([f(x).diff(x), f(x)]) Out[68]: ⎛ x ⎞ d (x + 1)⋅f(x) + ⎜──── + 1⎟⋅──(f(x)) ⎝f(x) ⎠ dx

For ODE pattern matching collecting on the derivative is more important than collecting on f(x).

eq.collect([f(x).diff(x)], func = cancel) solves the problem for preprocess for all solvers including a second order Liouville solver for which I will open a PR separately.

oscarbenjamin · 2020-03-18T19:28:29Z

sympy/solvers/ode/single.py

+                        factor = exp(Integral(factor).doit())
+                        m*= factor
+                        n*= factor
+                        self._wilds_match[P] = m.subs(y,fx)


There should be spaces after commas and on both sides of something like *=. See PEP 8.

…ction with FirstExact class

tnzl · 2020-03-20T04:23:38Z

I'm not sure the global y is needed at all because we can integrate with functions as integration variables:
In [5]: Integral(f(x), (f(x), 0, 1))                                                                                              
Out[5]: 
1             
⌠             
⎮ f(x) d(f(x))
⌡             
0             

In [6]: Integral(f(x), (f(x), 0, 1)).doit()                                                                                       
Out[6]: 1/2
Maybe it's just not necessary to substitute f(x) for y in the first place. Can the hack be removed like that?

The general solution contains integrals that require f(x) and x to be treated as independent of each other.
This is the reason for substituting f(x) with y till the step where integration is done. At present there is no other way to pass y to _handle_Integral.

oscarbenjamin · 2020-03-20T13:28:05Z

At present there is no other way to pass y to _handle_Integral.

Can the general solution be returned as a Subs?

e.g.:

In [45]: Subs(Integral(x*y, y), y, f(x))                                                                                          
Out[45]: 
⎛⌠       ⎞│      
⎜⎮ x⋅y dy⎟│      
⎝⌡       ⎠│y=f(x)

In [46]: Subs(Integral(x*y, y), y, f(x)).doit()                                                                                   
Out[46]: 
   2   
x⋅f (x)
───────
   2

tnzl · 2020-03-21T11:13:17Z

At present there is no other way to pass y to _handle_Integral.

Can the general solution be returned as a Subs?

e.g.:

In [45]: Subs(Integral(x*y, y), y, f(x))                                                                                          
Out[45]: 
⎛⌠       ⎞│      
⎜⎮ x⋅y dy⎟│      
⎝⌡       ⎠│y=f(x)

In [46]: Subs(Integral(x*y, y), y, f(x)).doit()                                                                                   
Out[46]: 
   2   
x⋅f (x)
───────
   2

The general solution must be returned in integral form. Integral would be handled by dsolve later.

oscarbenjamin · 2020-03-21T11:43:18Z

The general solution must be returned in integral form. Integral would be handled by dsolve later.

Can it be returned as Subs(Integral)?

tnzl · 2020-03-21T15:36:01Z

Yes!!! This works perfectly.

collected on f(x).diff(x) before matching pattern Updated wild variable P of FirstExact Removed unused imports and variables

oscarbenjamin · 2020-03-28T10:47:28Z

sympy/solvers/ode/single.py

+            if simplify(m) != 0:
+                m = m.subs(fx,y)
+                n = n.subs(fx,y)
+                numerator= simplify(m.diff(y)-n.diff(x))


I still think that all of this code can be simplified somehow. Are all these calls to simplify necessary? In general we try to avoid calling simplify in library code because it can be slow. It is better to call specific simplify functions like factor, cancel etc. if we know what is needed.

Maybe we can just use something like if m.is_zero rather than if simplify(m) != 0.

simplify is called only before critical comparisons for converting to exact ODE, therefore removing doesn't seem possible. is_zero would not work here because it doesn't simplify expression before returning. I think this is the best way to simplify at present.

I would rather not call simplify during the matching code so it might be better just to be inconclusive in some cases.

Can you give an example of what m and n might be here so that simplify is needed?

is_zero won't work here because it doesn't simplify the expression before return. Here, simplify is called only before critical conditions to convert to exact ODE therefore simplify can't be removed.
At present I think simplify is the best way to simplify expressions here.

Can you give the example of what m and n might be? There is almost always a better option then using generic simplify.

oscarbenjamin · 2020-03-28T10:49:13Z

sympy/solvers/ode/single.py

+        except NotImplementedError:
+            # Differentiating the coefficients might fail because of things
+            # like f(2*x).diff(x).  See issue 4624 and issue 4719.
+            pass


This try block is too broad. There is too much code in between the try and the except. We should only catch exceptions around the narrowest block of code possible (ideally a single function call). Where does the NotImplemented error come from? Maybe the try/except can just be removed.

Are the tests failing because of my master branch being out of sync with sympy's master?

oscarbenjamin · 2020-03-28T14:33:05Z

The tests failed because of #18978

tnzl · 2020-03-29T12:18:57Z

@oscarbenjamin can you please guide me on why the tests are failing? All tests seems to have passed.

oscarbenjamin · 2020-03-29T12:30:30Z

I think it was just a network problem. I've restarted the tests that stalled.

codecov · 2020-03-29T13:23:20Z

Codecov Report

Merging #18887 into master will increase coverage by 0.017%.
The diff coverage is 96.551%.

@@              Coverage Diff              @@
##            master    #18887       +/-   ##
=============================================
+ Coverage   75.777%   75.795%   +0.017%     
=============================================
  Files          647       647               
  Lines       168623    168636       +13     
  Branches     39734     39732        -2     
=============================================
+ Hits        127778    127818       +40     
+ Misses       35280     35260       -20     
+ Partials      5565      5558        -7

tnzl · 2020-03-30T11:15:35Z

Thanks. Are there any other suggestions that you want me to implement here?

oscarbenjamin · 2020-03-30T11:17:47Z

You haven't addressed the comments above

oscarbenjamin · 2020-04-26T00:38:07Z

@tnzl what is the status of this?

czgdp1807 · 2020-05-02T17:47:21Z

@tnzl Are you still working on this?

czgdp1807 · 2021-01-02T07:24:08Z

@oscarbenjamin Can we close this or can it be taken over by some other contributor in future?

oscarbenjamin · 2021-01-02T10:46:56Z

This is still needed so it would be good if someone could take this over.

tnzl added 3 commits March 16, 2020 23:48

Added FirstExact solver to single.py and refactored classify_ode() an…

0523322

…d _handle_Integral() functions in ode.py

Integral in sol left for _handle_Integral(..). Removed .doit() from _…

89e87d6

…get_general_solution() of FirstExact class.

Minor updates.

96ca1c0

oscarbenjamin reviewed Mar 17, 2020

View reviewed changes

czgdp1807 added solvers.dsolve solvers labels Mar 17, 2020

tnzl added 3 commits March 18, 2020 15:54

Changed inheritance to SinglePatternODESolver

e3c47bf

Removed trailing white spaces

36184e1

Updating docstrings of FirstExact

233cafb

oscarbenjamin reviewed Mar 18, 2020

View reviewed changes

Updated /doc/src/modules/solvers/ode.rst to replace ode_1st_exact fun…

3be219f

…ction with FirstExact class

tnzl force-pushed the master branch from b6a854a to 3be219f Compare March 19, 2020 03:42

Global y hack removed

f6d4d2c

tnzl added 2 commits March 22, 2020 00:24

Variable names made consistent

e8c0c02

Removed unused variables

f9c8f09

Set the order of FirstExact solver

7b148fd

collected on f(x).diff(x) before matching pattern Updated wild variable P of FirstExact Removed unused imports and variables

tnzl force-pushed the master branch from 57171c9 to 7b148fd Compare March 28, 2020 10:08

oscarbenjamin reviewed Mar 28, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master'

4af0408

czgdp1807 added the Could Close label May 2, 2020

habitzreuter mentioned this pull request Mar 19, 2021

Refactor Exact ODE solver into a class #21124

Merged

github-actions bot added the Merge conflict label Oct 26, 2022

Exact ODE solver class added #18887

Are you sure you want to change the base?

Exact ODE solver class added #18887

Conversation

tnzl commented Mar 17, 2020 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Mar 17, 2020 • 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

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

oscarbenjamin commented Mar 18, 2020

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

tnzl Mar 21, 2020 • 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

tnzl commented Mar 20, 2020

oscarbenjamin commented Mar 20, 2020 • edited

tnzl commented Mar 21, 2020

oscarbenjamin commented Mar 21, 2020

tnzl commented Mar 21, 2020 • 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

tnzl Mar 28, 2020 • 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

oscarbenjamin commented Mar 28, 2020

tnzl commented Mar 29, 2020 • edited

oscarbenjamin commented Mar 29, 2020

codecov bot commented Mar 29, 2020 • edited

Codecov Report

tnzl commented Mar 30, 2020

oscarbenjamin commented Mar 30, 2020

oscarbenjamin commented Apr 26, 2020

czgdp1807 commented May 2, 2020

czgdp1807 commented Jan 2, 2021

oscarbenjamin commented Jan 2, 2021

tnzl commented Mar 17, 2020 •

edited

sympy-bot commented Mar 17, 2020 •

edited

tnzl Mar 21, 2020 •

edited

oscarbenjamin commented Mar 20, 2020 •

edited

tnzl commented Mar 21, 2020 •

edited

tnzl Mar 28, 2020 •

edited

tnzl commented Mar 29, 2020 •

edited

codecov bot commented Mar 29, 2020 •

edited