-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
[WIP] Rational Riccati solver #21459
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
Conversation
|
✅ Hi, I am the SymPy bot (v161). 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.9. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
|
I haven't looked through this in detail but it's good to see this coming. I would put the algorithm in a separate file like The code needs to give much more explanation about the algorithm. Imagine someone in future who hasn't read the papers on which the algorithm is based trying to fix a simple bug. It's too much for them to go read a PhD thesis to understand what your code is doing. The top of the ricatti.py module should outline the algorithm and should also make it very clear what the reference for the algorithm is. Any assumptions you make and anything that you are not 100% sure about should be clearly documented within the code so that someone in future can understand what you were thinking when you wrote that code (e.g. if the assumption turns out to be incorrect in some cases). |
🟠Hi, I am the SymPy bot (v161). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it. This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75. The following commits add new files:
If these files were added/deleted on purpose, you can ignore this message. |
The basic structure looks okay. I'm going to hold off on detailed comments for now because there is one big question that needs to be resolved which is whether this algorithm should be formulated in terms of Poly rather than Expr. There are lots of places which are using cancel, together, Poly expressions are always in a canonical form which simplifies a lot of things and makes algebra more robust, predictable and efficient for an algorithm such as this. Currently this uses If this can be written in terms of Poly and if it is better to do so then it is better to make that decision now as it will only be harder later. This is especially important if this is just the first of many related algorithms that should all share code and use the same kind of internal representation. There are certain limitations of Poly such as the fact that symbolic powers of the independent variable would not be possible e.g. We can discuss this when we meet. |
|
I think using As for Laurent series, I'll have to see how to get the coefficients in an efficient way without using the inbuilt function. I think that there is something mentioned in Smith's implementation of Kovavic's algorithm about finding the coefficients. |
|
If we use Poly, these helper functions in risch will be useful. sympy/sympy/integrals/risch.py Lines 811 to 886 in 31d469a
|
|
To make the substitution In [11]: f = (x**2 - 1)/(x**2 + 1)
In [12]: f
Out[12]:
2
x - 1
──────
2
x + 1
In [13]: num, den = [Poly(e, x) for e in f.as_numer_denom()]
In [14]: num
Out[14]: Poly(x**2 - 1, x, domain='ZZ')
In [15]: den
Out[15]: Poly(x**2 + 1, x, domain='ZZ')
In [18]: num.transform(Poly(1, x), Poly(x, x))
Out[18]: Poly(-x**2 + 1, x, domain='ZZ')
In [19]: den.transform(Poly(1, x), Poly(x, x))
Out[19]: Poly(x**2 + 1, x, domain='ZZ')
In [20]: num.transform(Poly(1, x), Poly(x, x)) / den.transform(Poly(1, x), Poly(x, x))
Out[20]:
2
1 - x
──────
2
x + 1If the degree of the numerator and denominator are different then some factors of |
I think this should work. In [3]: f = (x**2 - 1)/x**3
In [4]: f.subs(x, 1/x)
Out[4]:
3 ⎛ 1 ⎞
x ⋅⎜-1 + ──⎟
⎜ 2⎟
⎝ x ⎠
In [5]: num, den = [Poly(e, x) for e in f.as_numer_denom()]
In [6]: num.transform(Poly(1, x), Poly(x, x)) / den.transform(Poly(1, x), Poly(x, x))
Out[6]:
2
1 - x
In [7]: (num.transform(Poly(1, x), Poly(x, x)) / den.transform(Poly(1, x), Poly(x, x)))*x**(den.degree(x) - num.degree(x))
Out[7]:
⎛ 2⎞
x⋅⎝1 - x ⎠ |
|
We were discussing yesterday that it's not possible to use |
For now you could just just use Expr for that part. I think it should be straight-forward to make a routine to compute the series for rational functions though. For example the coefficients in the Laurent part of the series can be calculated like: In [12]: f = (1 + x)/(4 - x**2)
In [13]: r1, r2 = roots(f.as_numer_denom()[1])
In [14]: f.series(x, r1)
Out[14]:
2 3 4 5
1 9 3⋅(x + 2) 3⋅(x + 2) 3⋅(x + 2) 3⋅(x + 2) 3⋅x ⎛ 6 ⎞
- ───────── + ── + ────────── + ────────── + ────────── + ────────── + ─── + O⎝(x + 2) ; x → -2⎠
4⋅(x + 2) 32 256 1024 4096 16384 64
In [15]: cancel(f*(x - r1)).subs(x, r1)
Out[15]: -1/4In general for the series you have a recursion relation: For the In [35]: cancel(f.subs(x, x+r1)*x)
Out[35]:
1 - x
─────
x - 4Now we want the Taylor series of this around In [39]: a = IndexedBase('a')
In [40]: Eq(1 - x, (x - 4)*Sum(a[m]*x**m, (m, 0, oo)))
Out[40]:
∞
___
╲
╲ m
1 - x = (x - 4)⋅ ╱ x ⋅a[m]
╱
‾‾‾
m = 0 Multiply out and rearrange the indices of the sums: Then equate coefficients of powers of a0 = -1/4
a1 = 3/16
a[m] = a[m-1]/4The part of this that would be potentially tricky though is needing to be able to compute in an extension field that includes the roots of the polynomials when the roots are not rational e.g. In [43]: f = (1 + x)/(2 - x**2)
In [44]: r1, r2 = roots(f.as_numer_denom()[1])
In [45]: f.series(x, r1)
Out[45]:
1 √2
- ─ + ──
2 4 √2 1 ⎛√2 1 ⎞ ⎛1 √2⎞ 2 ⎛ √2 1 ⎞ 3 ⎛ 1 √2⎞
──────── + ── + ─ + ⎜── + ──⎟⋅(x + √2) + ⎜── + ──⎟⋅(x + √2) + ⎜─── + ───⎟⋅(x + √2) + ⎜─── + ───⎟⋅
x + √2 8 8 ⎝32 16⎠ ⎝64 64⎠ ⎝256 128⎠ ⎝512 512⎠
4 ⎛ √2 1 ⎞ 5 ⎛ 6 ⎞
(x + √2) + ⎜──── + ────⎟⋅(x + √2) + O⎝(x + √2) ; x → -√2⎠
⎝2048 1024⎠ To handle something like that you want the domain to be In [49]: f = (1 + x)/(2 - x**2)
In [50]: num, den = [Poly(e, x) for e in f.as_numer_denom()]
In [51]: r1, r2 = roots(den)
In [52]: r1
Out[52]: -√2
In [55]: Poly(r1, x)
Out[55]: Poly(-sqrt(2), x, domain='EX')
In [56]: Poly(r1, x, extension=True)
Out[56]: Poly(-sqrt(2), x, domain='QQ<sqrt(2)>')
In [57]: r1p = Poly(r1, x, extension=True)
In [58]: num.transform(x + r1p, 1)
Out[58]: Poly(x + 1 - sqrt(2), x, domain='QQ<sqrt(2)>')
In [59]: den.transform(x + r1p, 1)
Out[59]: Poly(-x**2 + 2*sqrt(2)*x, x, domain='QQ<sqrt(2)>')
In [60]: cancel(f.subs(x, x + r1)*x)
Out[60]:
-x - 1 + √2
───────────
x - 2⋅√2 In general we might want to use |
|
One of the advantages of having separate functions for the different steps of the algorithm is that we can make separate tests for them. There should be direct tests for the functions in ricatti.py separately from the dsolve tests. |
Should this be in a separate file, like |
Yes |
|
I've made most of the changes that you suggested. I will now work on creating the |
|
@oscarbenjamin Anything else that is left to do here? Right now, only the sphinx build is failing. I think its because I should add a newline before the docstring ends. I will change that along with any others if required. |
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[ed9a550f] [fb2d2999]
<sympy-1.8^0>
+ 954±3ms 1.62±0s 1.70 dsolve.TimeDsolve01.time_dsolve
- 7.83±0.01s 3.73±0s 0.48 integrate.TimeIntegrationRisch02.time_doit_risch(100)
- 13.3±0.8ms 8.38±0.02ms 0.63 logic.LogicSuite.time_load_file
+ 69.5±4μs 2.27±0.01ms 32.59 matrices.TimeDiagonalEigenvals.time_eigenvals
- 4.56±0.01ms 2.46±0ms 0.54 solve.TimeRationalSystem.time_linsolve(10)
- 922±2μs 574±2μs 0.62 solve.TimeRationalSystem.time_linsolve(5)
- 1.07±0ms 704±5μs 0.66 solve.TimeSparseSystem.time_linsolve_eqs(10)
- 2.02±0ms 1.28±0ms 0.63 solve.TimeSparseSystem.time_linsolve_eqs(20)
- 2.99±0.01ms 1.87±0ms 0.63 solve.TimeSparseSystem.time_linsolve_eqs(30)
Full benchmark results can be found as artifacts in GitHub Actions |
|
Looks good. Let's get this in! |
|
Actually it would be good to expand the release note a bit. Can you expand the release note a little to explain what the significance of this solver is: |
|
It would be good to add some benchmark cases for the algorithm to the benchmarks repo: |
I've added some notes for it. Let me know if something else must also be mentioned.
Sure. Should I put it in |
Yes, I think so. It would be good to test both how long dsolve takes but also how long the solver takes without dsolve. |
|
Hi, |
|
The Riccati solver can only find rational solutions. To solve this ODE, the kovacic method or something similar needs to be implemented as well. |
|
Is such an implementation planned in the near future? That would be really useful. |
|
Any progress in that direction? Thanks a lot. Sympy is great! |
|
There hasn't been any progress. I think we need to spell out somewhere what should be done so that it can be maybe another GSOC project. |
|
Thanks a lot!
… Le 30 juin 2022 à 14:51, Oscar Benjamin ***@***.***> a écrit :
There hasn't been any progress. I think we need to spell out somewhere what should be done so that it can be maybe another GSOC project.
—
Reply to this email directly, view it on GitHub <#21459 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APKF3JFGQAQ3ROK3OQO5L23VRWJ4FANCNFSM44WHY3ZQ>.
You are receiving this because you commented.
|
This PR introduces a Rational Riccati ODE solver. This code can also be used to solve a subclass of 2nd order ODEs solvable by Kovacic's Algorithm
References to other Issues or PRs
Partial Fix #19183, #21503
Brief description of what is fixed or changed
The algorithm present here is implemented. The examples used to test are here.
Other comments
I will soon add the Kovacic solver.
Release Notes