refactor const homogenous hints by Mohitbalwani26 · Pull Request #21654 · sympy/sympy

Mohitbalwani26 · 2021-06-23T07:36:30Z

References to other Issues or PRs

SEE #18348

Brief description of what is fixed or changed

Other comments

Release Notes

NO ENTRY

sympy-bot · 2021-06-23T07:36:31Z

✅

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


#### 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 -->

Mohitbalwani26 · 2021-06-23T07:41:25Z

+        if not reduced_eq:
+            reduced_eq = self.eq
+        return reduced_eq
+


@oscarbenjamin This snippet is kept as separate function for now because when I keep it inside preprocess_eq, other solvers( pattern matching ones) fail to classify many examples as everytime the highest order coeff is removed. example : f(x).diff(x)*f(x) + f(x)*f(x) - x*f(x). If you could please help in this, how this should be handled as this manipulation is required for const coeff hints.

I think this is fine.

Mohitbalwani26 · 2021-06-23T07:45:40Z

+        gsol = Eq(f(x), gsol)
+        self.r.update({'list': gensols, 'sol': gsol, 'simpliy_flag': simplify_flag})
+        gsol = self._solve_undetermined_coefficients(self.r)
+        if simplify_flag:


Also, this raises an exception

File "/home/runner/work/sympy/sympy/sympy/solvers/ode/single.py", line 2691, in _get_general_solution gsol = self._solve_undetermined_coefficients(self.r) File "/home/runner/work/sympy/sympy/sympy/solvers/ode/single.py", line 2668, in _solve_undetermined_coefficients coeffvals = solve(list(coeffsdict.values()), coefflist) raise GeneratorsNeeded( sympy.polys.polyerrors.GeneratorsNeeded: can't initialize from 'dict' without generators

whereas when I used inheritance, it was working fine.

oscarbenjamin · 2021-06-24T10:54:14Z

+        >>> from sympy.abc import x
+        >>> from sympy.solvers.ode.ode import _nth_linear_match
+        >>> f = Function('f')
+        >>> _nth_linear_match(f(x).diff(x, 3) + 2*f(x).diff(x) +


These examples should be updated to show how to use the method here

oscarbenjamin · 2021-06-24T10:57:19Z

+    def get_linear_coefficients(self, eq, func, order):
+        r"""
+        Matches a differential equation to the linear form:
+
+        .. math:: a_n(x) y^{(n)} + \cdots + a_1(x)y' + a_0(x) y + B(x) = 0
+
+        Returns a dict of order:coeff terms, where order is the order of the


Can this function just use the _lineq2dict function defined here:

sympy/sympy/polys/matrices/linsolve.py

Lines 171 to 172 in 6a57794

def _lin_eq2dict(a, symset):

"""Efficiently convert a linear equation to a dict of coefficients"""

>>> _lin_eq2dict(eq, {f(x),f(x).diff(x),f(x).diff(x,2),f(x).diff(x,3)}) (x - sin(x), {f(x): -1, Derivative(f(x), x): cos(x) + 2, Derivative(f(x), (x, 2)): x, Derivative(f(x), (x, 3)): 1})

This also looks fine, only output format is changed. so should I change to use this function?

Yes. that should simplify it a bit.

The docstring should be rewritten just to say that it gets the coefficients of the ODE. Is it more convenient for the calling code to have a dict with e.g. f(x) as the key or just a list of coefficients in order of derivatives?

I think dictionary is good as it provides more readability. If you want me to change to list, I will do that

@oscarbenjamin _lin_eq2dict() does not check whether the given ODE is linear. so should I keep nth_linear_match code only?

>>> eq=f(x).diff(x, 3) + 2*f(x).diff(x) +x*f(x).diff(x, 2) + cos(x)*f(x).diff(x) + x - f(x) -sin(f(x)) >>> _nth_linear_match(eq, f(x), 3) None >>> _lin_eq2dict(eq,{f(x),f(x).diff(x),f(x).diff(x,2),f(x).diff(x,3)}) (x - sin(f(x)), {f(x): -1, Derivative(f(x), x): cos(x) + 2, Derivative(f(x), (x, 2)): x, Derivative(f(x), (x, 3)): 1})

Oh yes, I forgot about that.

You could check if the rhs or any of the values of the dict has f(x) like:

In [1]: sin(f(x)).has(f(x)) Out[1]: True

oscarbenjamin · 2021-06-24T11:00:33Z

        Bernoulli, SingleODEProblem, SingleODESolver, RiccatiSpecial,
        SecondNonlinearAutonomousConserved, FirstExact, Liouville, Separable,
        SeparableReduced, HomogeneousCoeffSubsDepDivIndep, HomogeneousCoeffSubsIndepDivDep,
-        HomogeneousCoeffBest, LinearCoefficients, NthOrderReducible, Hypergeometric2nd)
+        HomogeneousCoeffBest, LinearCoefficients, NthOrderReducible, Hypergeometric2nd,
+        NthConstCoeffHomogen, NthConstCoeffVar, NthConstCoeffUndet)


We should keep the naming consistent. None of the other solvers has abbreviations like Undet. Also the other class names correspond directly to the hint names except in camel case rather than snake case.

NthLinearConstantCoeffUndeterminedCoefficients but wouldn't it be too long as a class name?

Hardly anyone ever needs to type the class name. The hint is what users have to type and is nth_linear_constant_coeff_undetermined_coefficients. It would have been good to make that shorter but it's too late for that now. For the class names consistency is more important than length.

oscarbenjamin · 2021-06-24T11:02:14Z

-        'sol': [Eq(f(x), (C1 + C2*x)*exp(x*r21) + (C10*sin(x*im(r24)) + C7*x*sin(x*im(r24)) + (
-        C8 + C9*x)*cos(x*im(r24)))*exp(x*re(r24)) + (C3*x*sin(x*im(r22)) + C6*sin(x*im(r22)
-        ) + (C4 + C5*x)*cos(x*im(r22)))*exp(x*re(r22)))],
+        'sol': [Eq(f(x), (C1 + C2*x)*exp(x*r21) + (-((C3 + C4*x)*sin(x*im(r22)))
+        + (C5 + C6*x)*cos(x*im(r22)))*exp(x*re(r22)) + (-((C7 + C8*x)*sin(x*im(r24)))
+        + (C10*x + C9)*cos(x*im(r24)))*exp(x*re(r24)))],


Why has this changed?

I think because of ordering of constants and simplification function is called inside solver earlier this was handled in odesimp.

Can we not just let odesimp handle this?

In odesimp there was explicit condition for the hints having nth_linear_const_coeff, with the help of global variable it was handling the simplification which we discussed in the meeting. But now that simplification is moved in the solver.

I still don't understand why we can't just let odesimp do all the simplification. If it can't figure out which expressions to collect on then maybe odesimp needs to be improved.

>>> eq=-x**4*sin(x - 1) + f(x) + Derivative(f(x), (x, 2)) [(0, 0, -1), (0, 0, 1)] #(multiplicity, reroot, imroot) sol = collect(sol, x**i*exp(reroot*x)*sin(abs(imroot)*x)) sol = collect(sol, x**i*exp(reroot*x)*cos(imroot*x)) sol = collect(sol, x**i*exp(reroot*x)) sol = powsimp(sol) expected_sol = Eq(f(x), C1*sin(x) + C2*cos(x) - x**5*cos(x - 1)/10 + x**4*sin(x - 1)/4 + x**3*cos(x - 1)/2 - 3*x**2*sin(x - 1)/4 - 3*x*cos(x - 1)/4)

The code snippet which I shared here gives Eq(f(x), C1*sin(x) + C2*cos(x) + x**2*(x**2 - 3)*sin(x - 1)/4 + x*(-2*x**4 + 10*x**2 - 15)*cos(x - 1)/20)

This discussion would be a lot quicker if you would communicate more clearly.

What is sol in this example?

sorry for the miscommunication.
Let's take the example eq=-8*f(x) + 12*Derivative(f(x), x) - 6*Derivative(f(x), (x, 2)) + Derivative(f(x), (x, 3)) the root is 2. so the collectterms (which is an array of tupe having (multiplicity, realroot, imaginaryPart)) will be [(2, 2, 0), (1, 2, 0), (0, 2, 0)].
general sol returned will be Eq(f(x), C1*exp(2*x) + C2*x*exp(2*x) + C3*x**2*exp(2*x)).
After doing collect on roots the expected simplified solution is Eq(f(x), (C1 + x*(C2 + C3*x))*exp(2*x)).
If it is still not clear, can we have discuss this on meet?

We can do that like:

In [24]: C1, C2, C3 = symbols('C1:4') In [25]: eq = Eq(f(x), C1*exp(2*x) + C2*x*exp(2*x) + C3*x**2*exp(2*x)).rhs In [26]: eq Out[26]: 2⋅x 2⋅x 2 2⋅x C₁⋅ℯ + C₂⋅x⋅ℯ + C₃⋅x ⋅ℯ In [27]: eq.collect(eq.atoms(exp)) Out[27]: ⎛ 2⎞ 2⋅x ⎝C₁ + C₂⋅x + C₃⋅x ⎠⋅ℯ In [28]: eq.collect(eq.atoms(exp), horner) Out[28]: 2⋅x (C₁ + x⋅(C₂ + C₃⋅x))⋅ℯ

Actually I prefer Out[27] though. I don't see why we would want to change it to Out[28].

Yes out[27] is achievable, but this was just one of the examples. I think I should commit and change the examples and then we can discuss further if the examples look simplified or not.

Mohitbalwani26 · 2021-06-26T16:50:53Z

@oscarbenjamin can you review this now?

oscarbenjamin · 2021-06-27T00:40:41Z

        SeparableReduced, HomogeneousCoeffSubsDepDivIndep, HomogeneousCoeffSubsIndepDivDep,
-        HomogeneousCoeffBest, LinearCoefficients, NthOrderReducible, Hypergeometric2nd)
+        HomogeneousCoeffBest, LinearCoefficients, NthOrderReducible, Hypergeometric2nd,
+        NthLinearConstantCoeffHomogeneous, NthLinearConstantCoeffVariationOfParameters, NthLinearConstantCoeffUndeterminedCoefficients)


This line is too long

oscarbenjamin · 2021-06-27T00:41:16Z

        eq = [eq]
-
-        # special simplification of the rhs
        if hint.startswith("nth_linear_constant_coeff"):


Do we still need to check the hint here? It would be better not to have simplification that is specific to one hint if possible.

yes, because the way collect is done is specifically for the solutions returned by nth_linear_constant_coeff due to roots of characterstic equation.
If we remove this if statement, It will change the form of solutions of other hints too as they already return solution solved for func.
Example:

>>> eq = (x*log(f(x)/x) - 2*x)*Derivative(f(x), x) + f(x) >>> dsolve(eq) # keeping this If condition Eq(f(x), -exp(C1)*LambertW(-x*exp(1 - C1))) >>> dsolve(eq) #removing If condition Eq(f(x), -exp(C1)*LambertW(-E*x*exp(-C1)))

Ideally we would apply the same simplification routines to the solutions from all solvers. For now we can leave this here but ultimately I think it should be removed.

Perhaps the best result would be to use the simpsol function from systems.py.

oscarbenjamin · 2021-06-27T00:42:50Z

-            # no reason.  We call powsimp to fix.
+            arr=eq[0].rhs.atoms(exp)
+            arr2=eq[0].rhs.atoms(sin)
+            arr3=eq[0].rhs.atoms(cos)


Can we just do rhs.atoms(exp, sin, cos) and then collect on that?

The reason I did separately was because we also want to do collect(expr, exp(x)*sin(x)) basically on all combinations of exp*sin and exp*cos

Does it make a different to the end result?

The ans is correct without it also, but it was combining some examples in a nicer way. If you want me to remove, I will do that

When you say "in a nicer way" it's hard for me to know what you mean without any examples.

okay, sure I will write them.

Here is the list of examples. Link

Which way round are you passing the arguments to simpsol? Does that make a difference? How is simpsol called in systems.py?

These examples in the doc were generated using simpsol(eq, constants, [x]) . In most of the examples it wasn't making any difference when simpsol was called like this simpsol(eq, [x], constants). In this comment only these two examples doesn't change when the order of arguments was swapped.

In systems.py simpsol is called like simpsol(sol, [t], constants, doit=doit) where t is Independent variable in the system of ODEs

oscarbenjamin · 2021-06-27T00:43:33Z

+from .subscheck import sub_func_doit
+from sympy.polys.matrices.linsolve import _lin_eq2dict
+from sympy.polys.solvers import PolyNonlinearError
 from .hypergeometric import equivalence_hypergeometric, match_2nd_2F1_hypergeometric, get_sol_2F1_hypergeometric, match_2nd_hypergeometric


This line is too long

oscarbenjamin · 2021-06-27T00:44:36Z

+        x = func.args[0]
+        symset = {Derivative(f(x), x, i) for i in range(order+1)}
+        try:
+            rhs,lhs_terms = _lin_eq2dict(eq, symset)


Space after comma

oscarbenjamin · 2021-06-27T00:47:41Z

         '1st_power_series',
         'lie_group',
-         'nth_linear_constant_coeff_homogeneous',
+        #  'nth_linear_constant_coeff_homogeneous',


If this is not supposed to match any more then this list should be deleted

Mohitbalwani26 · 2021-06-30T07:06:12Z

@oscarbenjamin PTAL. Is there anything which needs to be changed?

oscarbenjamin · 2021-06-30T13:24:33Z

Looks good.

refactor const homogenous hints

2bb63bd

Mohitbalwani26 requested a review from oscarbenjamin as a code owner June 23, 2021 07:36

Mohitbalwani26 commented Jun 23, 2021

View reviewed changes

Mohitbalwani26 added 2 commits June 23, 2021 21:44

fix poly error exception

c8d2caa

fix unexpanded equation case

de72782

oscarbenjamin reviewed Jun 24, 2021

View reviewed changes

Mohitbalwani26 added 2 commits June 24, 2021 23:54

change class name and use _lin_eq2dict

c111fb7

fix code quality

64a855d

oscarbenjamin added the solvers.dsolve label Jun 25, 2021

update the get_linear_coeff code

8b18556

oscarbenjamin reviewed Jun 27, 2021

View reviewed changes

keep simplification code in solver and add comment

de4614d

Mohitbalwani26 force-pushed the refactor_const_homogen branch from 66cfbec to de4614d Compare June 29, 2021 18:11

Mohitbalwani26 mentioned this pull request Jun 29, 2021

dsolve 'nth_linear_constant_coeff_variation_of_parameters_Integral' hint does not return an integral #21688

Open

oscarbenjamin merged commit 6a5c741 into sympy:master Jun 30, 2021

Mohitbalwani26 deleted the refactor_const_homogen branch July 3, 2021 10:49

	def _lin_eq2dict(a, symset):
	"""Efficiently convert a linear equation to a dict of coefficients"""

Uh oh!

Uh oh!

Conversation

Mohitbalwani26 commented Jun 23, 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 23, 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!

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!

Mohitbalwani26 commented Jun 26, 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