refactor euler solvers by Mohitbalwani26 · Pull Request #21692 · sympy/sympy

Mohitbalwani26 · 2021-06-30T15:15:36Z

References to other Issues or PRs

SEE #18348

Brief description of what is fixed or changed

Refactor Euler solvers and cleanup the constant coff code from ode.py

Other comments

Release Notes

NO ENTRY

sympy-bot · 2021-06-30T15:15:44Z

✅

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.

No release notes entry will be added for this pull request.

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. -->
SEE #18348 

#### Brief description of what is fixed or changed
Refactor Euler solvers and cleanup the constant coff code from ode.py

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

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 -->
NO ENTRY
<!-- END RELEASE NOTES -->

oscarbenjamin · 2021-06-30T23:04:26Z

 def test_undetermined_coefficients_match():
-    assert _undetermined_coefficients_match(g(x), x) == {'test': False}
-    assert _undetermined_coefficients_match(sin(2*x + sqrt(5)), x) == \
+    F = lambda eq: NthLinearConstantCoeffUndeterminedCoefficients(SingleODEProblem(eq, f(x), x))._undetermined_coefficients_match(eq,x)


It's helpful during refactoring if the tests are changed as little as possible so we can be clear that the mostly unchanged tests still pass. This could be:

def _undetermined_coefficients_match(eq, x): problem = SingleODEProblem(eq, f(x), x) solver = NthLinearConstantCoeffUndeterminedCoefficients(problem) return solver. _undetermined_coefficients_match(eq, x)

Then the rest of the testing code would not need to be changed.

oscarbenjamin · 2021-07-01T19:07:40Z

+        >>> f = Function('f')
+        >>> eq = 9*x*exp(x) + exp(-x)
+        >>> obj = NthLinearConstantCoeffUndeterminedCoefficients(SingleODEProblem(eq,f(x),x))
+        >>> obj._undetermined_coefficients_match(eq,x)


There should be a space after a comma (here and everywhere else)

oscarbenjamin · 2021-07-01T19:07:57Z

-        >>> from sympy import log, exp
-        >>> from sympy.solvers.ode.ode import _undetermined_coefficients_match
+        >>> from sympy import log, exp, Function
+        >>> from sympy.solvers.ode.single import NthLinearConstantCoeffUndeterminedCoefficients,SingleODEProblem


This line is too long

oscarbenjamin · 2021-07-01T19:08:40Z

+        >>> eq = log(x)
+        >>> obj = NthLinearConstantCoeffUndeterminedCoefficients(SingleODEProblem(eq,f(x),x))
+        >>> obj._undetermined_coefficients_match(eq,x)
        {'test': False}


It's probably better to give an example using dsolve here.

This docstring is just for getting trial set method. For examples of undet_coeff solver the docstring at class level has examples with dsolve.

oscarbenjamin · 2021-07-01T19:09:35Z

            sol = collect(sol, x**i*exp(reroot*x))
        sol = powsimp(sol)
-        return [Eq(f(x), sol)]
+        return Eq(f(x), sol)


Returning anything other than a list should fail here. Is there some code somewhere else that checks for this? Why didn't this fail before?

Currently test_single.py handles both type of solutions with list as well as without as some of the solvers are yet to be refactored. After refactoring I will remove the conditional part from test suite so that it should expect list of solution from every solver.

oscarbenjamin · 2021-07-01T19:11:03Z

+        return gensols
+
+
+    def _get_general_solution(self, *, simplify_flag: bool = True):


Only one blank line here

oscarbenjamin · 2021-07-01T19:13:09Z

+
+    def _get_general_solution(self, *, simplify_flag: bool = True):
+        self._get_sols(self.r)
+        return [self.gsol]


Shouldn't this just be return self._get_sols()?

It would be better if the methods in these classes don't use internal variables like this. We should minimise instance attributes.

Why is _get_sols a different method from _get_general_solution?

_get_sols() is the method which calcutates the roots of characteristic equation. And to form that we need match_object.
self.r was kept as an argument because this method is also used by euler variation of parameters and euler undetermined coeffs so we need to pass their match object to this.

Rather than passing the match object it would be better if this had names arguments like roots, trialset or something.

Why can't the vop and undet classes just use the euler class get_general_solution method?

Why can't the vop and undet classes just use the euler class get_general_solution method?

because get_general_solution will return a equation where as in vop and undet we just want the list of roots of characterstic equation.

oscarbenjamin · 2021-07-01T19:19:20Z

+        chareq, symbol = S.Zero, Dummy('x')
+
+        for i in r.keys():
+            if not isinstance(i, str) and i >= 0:


If r is a dict then it isn't necessary to call keys here (the dict is already iterable).

Why would i be a str?

r can have key named 'trialset' which will be used for undetermined coefficient.

It would be better to store trialset as a separate attribute rather than mix it up with the roots:

self.roots = ... self.trialset = ...

oscarbenjamin · 2021-07-01T19:22:29Z

Side note: these Euler solvers take up 400 lines that would be reduced to almost nothing if the ODE was simply solved by substitution.

oscarbenjamin · 2021-07-02T14:00:05Z

        fx = self.ode_problem.func
        x = self.ode_problem.sym
-        (C1,) = self.ode_problem.get_numbered_constants(num=1)
+        (C1, ) = self.ode_problem.get_numbered_constants(num=1)


This space should not be here

oscarbenjamin · 2021-07-02T14:00:43Z


    These kinds of differential equations can be solved in a general way.  The
-    integrating factor `e^{\int P(x) \,dx}` will turn the equation into a
+    integrating factor `e^{\int P(x) \, dx}` will turn the equation into a


The \, in latex is different from other commas since it represents a small space. I don't think that we need to also add a space after this.

oscarbenjamin · 2021-07-02T14:01:14Z


    .. math:: y' + F\left(\!\frac{a_1 x + b_1 y + c_1}{a_2 x + b_2 y +
-                c_2}\!\right) = 0\text{,}
+                c_2}\!\right) = 0\text{, }


This space should not be here

oscarbenjamin · 2021-07-02T14:03:40Z

It looks like you've done a find and replace to add spaces after commas but some of these places shouldn't be changed so can you check through those?

Mohitbalwani26 · 2021-07-02T14:06:39Z

It looks like you've done a find and replace to add spaces after commas but some of these places shouldn't be changed so can you check through those?

Yes I did that. I will check once. Also I am trying to move all the reusable functions to another file so that classes look minimal and kind of wrapper for them.

sympy-bot · 2021-07-02T17:39:31Z

🟠

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:

2eb490e:
- sympy/solvers/ode/euler_nonhomogen_helpers.py

If these files were added/deleted on purpose, you can ignore this message.

Mohitbalwani26 · 2021-07-02T22:04:58Z

@oscarbenjamin does this way seem good?
Also if yes then what should be the name of euler_nonhomogen_helpers.py?

oscarbenjamin · 2021-07-02T22:34:53Z

@oscarbenjamin does this way seem good?
Also if yes then what should be the name of euler_nonhomogen_helpers.py?

Yes, I think so. I think it should just be called nonhomogeneous.py. Other functions like variation of parameters etc can go there later.

Mohitbalwani26 · 2021-07-03T05:36:25Z

Yes, I think so. I think it should just be called nonhomogeneous.py. Other functions like variation of parameters etc can go there later.

So should I again move functions like solve_variation_of_param , solve_unde_coeff back to classes?

Currently all the functions which are there in new file they all are used by more than 1 solver. If we again keep any of them into solver, we will have to create instance of that class into another.

So what all functions should I keep in classes back?

oscarbenjamin · 2021-07-03T09:28:53Z

+                gensols.append(sin_form)
+    return gsol, gensols
+
+def _solve_variation_of_parameters(eq, func, roots, homogen_sol, order, match_obj, simplify_flag=True):


Two blank lines

oscarbenjamin · 2021-07-03T09:29:52Z

+        psol = trigsimp(psol, deep=True)
+    return Eq(f(x), homogen_sol.rhs + psol)
+
+def _get_const_characterstic_eq_sols(r, func, order):


characteristic

oscarbenjamin · 2021-07-03T09:31:14Z

So should I again move functions like solve_variation_of_param , solve_unde_coeff back to classes?

No I think this is fine. Eventually these functions should be made into something that it is more usefully reusable though.

oscarbenjamin · 2021-07-03T12:24:26Z

@@ -0,0 +1,463 @@
+from collections import defaultdict


There should be some kind of comment or module level docstring at the top of the module explaining what it is.

oscarbenjamin · 2021-07-03T12:54:41Z

+    return False
+
+
+def _get_euler_characterstic_eq_sols(eq, func, match_obj):


The functions in this file should have at least a one line docstring explaining what they are as well as comments explaining why they are here and what they are used for.

oscarbenjamin · 2021-07-03T20:22:06Z

+def _get_euler_characteristic_eq_sols(eq, func, match_obj):
+    r"""
+    It returns the solution of homogeneous part of the linear euler ODE and
+    the list of roots of characteristic equation.


Don't use "It" here. Just "Returns ..." is fine.

This doesn't really explain the function very well. What is the match_obj parameter supposed to be for example?

oscarbenjamin · 2021-07-03T20:24:08Z

+Functions that are for internal use:
+
+1) _test_term(coeff, func, order) - It takes coefficient of partiular term, function and
+    order to which it is associated. It returns whether given term matches with linear euler ODEs.


I don't think that you need to detail each function at the top here. These kinds of comments/description are better placed in or immediately before the function.

This module level docstring just needs to explain the general purpose of the module.

oscarbenjamin · 2021-07-04T22:01:47Z

    def _wilds(self, f, x, order):
        fy = Wild('fy', exclude=[0, f(x).diff(x), f(x).diff(x, 2)])
-        return (fy,)
+        return (fy, )


There shouldn't be a space here

Okay, will remove it in next PR.

oscarbenjamin · 2021-07-04T22:05:06Z

Okay, looks good.

refactor euler solvers

3d24590

Mohitbalwani26 requested a review from oscarbenjamin as a code owner June 30, 2021 15:15

oscarbenjamin reviewed Jun 30, 2021

View reviewed changes

oscarbenjamin added the solvers.dsolve label Jun 30, 2021

minimize change in tests

efe503c

Mohitbalwani26 requested a review from oscarbenjamin July 1, 2021 18:40

oscarbenjamin reviewed Jul 1, 2021

View reviewed changes

fix long lines and space after comma

d988a9b

oscarbenjamin reviewed Jul 2, 2021

View reviewed changes

move reusable funtions to helper file

2eb490e

fix unreferenced variable

ce0f808

rename file

2f59982

oscarbenjamin reviewed Jul 3, 2021

View reviewed changes

fix blank lines

9cb786b

oscarbenjamin reviewed Jul 3, 2021

View reviewed changes

add some documentation at the top of file

f90e717

Mohitbalwani26 requested a review from oscarbenjamin July 3, 2021 18:57

oscarbenjamin reviewed Jul 3, 2021

View reviewed changes

Mohitbalwani26 added 2 commits July 4, 2021 03:04

update docstring

6e40729

update docstring

d943d0a

Mohitbalwani26 requested a review from oscarbenjamin July 4, 2021 11:25

oscarbenjamin reviewed Jul 4, 2021

View reviewed changes

oscarbenjamin merged commit eed6198 into sympy:master Jul 4, 2021

Mohitbalwani26 deleted the refactor_euler branch July 5, 2021 05:59

		return gensols


		def _get_general_solution(self, *, simplify_flag: bool = True):

		return False


		def _get_euler_characterstic_eq_sols(eq, func, match_obj):

Uh oh!

Uh oh!

Conversation

Mohitbalwani26 commented Jun 30, 2021

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

Uh oh!

sympy-bot commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin commented Jul 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin commented Jul 2, 2021

Uh oh!

Mohitbalwani26 commented Jul 2, 2021

Uh oh!

sympy-bot commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟠

Uh oh!

Mohitbalwani26 commented Jul 2, 2021

Uh oh!

oscarbenjamin commented Jul 2, 2021

Uh oh!

Mohitbalwani26 commented Jul 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin commented Jul 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sympy-bot commented Jul 2, 2021 •

edited

Loading