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
refactor lie group solver #21722
refactor lie group solver #21722
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.
Click here to see the pull request description that was parsed.
|
🟠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. |
6e3633c
to
590289b
Compare
@oscarbenjamin can you please review this? |
@@ -0,0 +1,1082 @@ | |||
from itertools import islice | |||
|
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 module needs a comment or module-level docstring that explains what it is
sympy/solvers/ode/lie_group.py
Outdated
This module contains the implementation of the lie_group hint for | ||
dsolve. Ideally any first order differential equation can be solved by lie_group. | ||
It contains various helper functions which apply different heuristics on the given equation | ||
and returns the solution. |
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 is not a good explanation. Many of the functions below have docstrings with some explanations and also references to papers on which this implementation is based. Those references should be included here ideally with some context like "this implements the algorithm from Y but also with heuristics from Z" or something like that.
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.
r"""
This module contains the implementation of the lie_group hint for
dsolve. Ideally any first order differential equation can be solved by lie_group.
It implements various heuristics.
References
-
abaco1_simple from E.S Cheb-Terrab, L.G.S Duarte and L.A,C.P da Mota, Computer Algebra
Solving of First Order ODEs Using Symmetry Methods, pp. 8 -
abaco1_product fromE.S. Cheb-Terrab, A.D. Roche, Symmetries and First Order ODE Patterns,
pp. 7 - pp. 8 -
abaco2_similar from E.S. Cheb-Terrab, A.D. Roche, Symmetries and First Order ODE Patterns,
pp. 10 - pp. 12 -
abaco2_unique_unknown from E.S. Cheb-Terrab, A.D. Roche, Symmetries and First Order ODE Patterns,
pp. 10 - pp. 12 -
abaco2_unique_general from E.S. Cheb-Terrab, A.D. Roche, Symmetries and First Order
ODE Patterns, pp. 10 - pp. 12 -
linear from Lie Groups and Differential Equations pp. 327 - pp. 329
-
function_sum from E.S. Cheb-Terrab, A.D. Roche, Symmetries and First Order
ODE Patterns, pp. 7 - pp. 8 -
bivariate from Lie Groups and Differential Equations pp. 327 - pp. 329
-
chi from E.S Cheb-Terrab, L.G.S Duarte and L.A,C.P da Mota, Computer Algebra
Solving of First Order ODEs Using Symmetry Methods, pp. 8
"""
@oscarbenjamin does this look ok?
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.
Lots of those references are repeated so the descriptions can be combined. Also this text doesn't really explain anything about what anything in the module does.
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.
Basically I have moved all the internal helper functions only and single.py
has matching code and general solution for the solver. So should I write one-two line for every function at the top because that also will be redundant as every function has its docstring.
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.
It's fine that each function has its own docstring. We just need some text that describes what this module contains from a high level.
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 module contains the implementation of the internal helper functions for the lie_group hint for
dsolve. These helper functions apply different heuristics on the given equation and return the solution. These functions are used by :py:meth:sympy.solvers.ode.single.LieGroup
References
-
abaco1_simple
,function_sum
andchi
are referenced from E.S Cheb-Terrab, L.G.S Duarte and L.A,C.P da Mota, Computer Algebra Solving of First Order ODEs Using Symmetry Methods, pp. 7 - pp. 8 -
abaco1_product
,abaco2_similar
,abaco2_unique_unknown
,linear
andabaco2_unique_general
are referenced from E.S. Cheb-Terrab, A.D. Roche, Symmetries and First Order ODE Patterns, pp. 7 - pp. 12 -
bivariate
from Lie Groups and Differential Equations pp. 327 - pp. 329
does this look better?
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.
Yes, that's better. Are there no authors listed for the third reference?
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.
No, The author isn't mentioned
if xieta: | ||
return xieta | ||
|
||
def lie_heuristic_abaco2_similar(match, comp=False): |
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.
There should be two blank lines between top-level functions/classes.
References to other Issues or PRs
SEE #18348
Brief description of what is fixed or changed
Other comments
Release Notes
NO ENTRY