Refactor dsolve to use classes for each solver

[oscarbenjamin]

References to other Issues or PRs

See #18348

Brief description of what is fixed or changed

Makes a start at refactoring dsolve to use classes for each solver. Refactors the nth_algebraic solver code to use the new classes.

Adds a new sympy.solvers.odesolvers package. I hope that code from the ode module can be refactored into multiple files because the ode module is large and messy. Eventually the idea would be to rename this package as ode.

Other comments

The design here is very much a work in progress but it works now and I hope that the work would be completed over many PRs (possibly as a GSOC project). I would like to get this one merged as a start so that contributors can try out moving other solvers to see what issues emerge.

Release Notes

NO ENTRY

[sympy-bot]

✅

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

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

See #18348

#### Brief description of what is fixed or changed

Makes a start at refactoring dsolve to use classes for each solver. Refactors the `nth_algebraic` solver code to use the new classes.

Adds a new `sympy.solvers.odesolvers` package. I hope that code from the `ode` module can be refactored into multiple files because the `ode` module is large and messy. Eventually the idea would be to rename this package as `ode`.

#### Other comments

The design here is very much a work in progress but it works now and I hope that the work would be completed over many PRs (possibly as a GSOC project). I would like to get this one merged as a start so that contributors can try out moving other solvers to see what issues emerge.

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

[oscarbenjamin]

I will rebuild this on top of #18414...

[vikrantmalik051]

@oscarbenjamin Can you please let me know when this PR and #18414 will be merged because I was going to add more classes in it.

[Mohitbalwani26]

@oscarbenjamin forv example if i want to implement method of undetermined coefficients than should i make PR to your forked repository or directly to sympy?

[oscarbenjamin]

I am hoping to move all the files around now in #18414. I need to resolve a problem with the docs before that can be merged.

You can open a PR for sympy that is based on this branch to work on this while that gets resolved. To do that check out this branch and then create a new branch from there. You can push that whole branch as a PR to sympy so the tests run on Travis. Make sure to say in the OP if the PR is not based off of master. If you do this then at some point you will need to copy out your changes manually into a new PR (which should be easy enough).

[vikrantmalik051]

Suggested change

	Noe you can create a SingleODEProblem instance and query its properties:
	Now you can create a SingleODEProblem instance and query its properties:

[oscarbenjamin]

Thanks. I'll remember that when I rewrite this but I won't commit it now as there's no need to rerun the tests before rewriting.

[Mohitbalwani26]

@oscarbenjamin can you provide link to instructions or something from which i get to know how i need to make PR as you suggested for this case?

[asmeurer]

I think the thing that needs the biggest refactoring in the ode module is classify_ode. Right now it's a giant function that has every hint listed in it in the function body. This means that for instance, even if you specify a hint to dsolve it has to call classify_ode to see if it really matches and to extract the coefficients, which means it has to check every possible match. Also various logic for rewriting or simplifying the ODE so that it can be matched is done on a case-by-case basis or not at all. Having a class for matching would allow factoring out common logic into a superclass, so for instance, most ODEs could just specify their general equation and the matcher would handle matching them.

Something like

class FirstLinearODE(ODESolver):
    def equation(func, x, n): # This is a function of n so it can work for the nth order solvers
        p, q = self.coeffs() # Returns the Wild coefficients
        return p*func + func.diff(x) - q

And then ODESolver has a match method that takes the equation and returns a match. Only advanced solvers like the algebraic solver would actually need to override the match method.

The ODE module also has a lot of nonsense keyword arguments that it passes around to keep from recomputing things. A lot of this is unnecessary if we have class instances. Some of it is completely unnecessary anyway.

[asmeurer]

I don't know if I would start with algebraic here. The algebraic solver works very differently from all the other solvers. I would start with the basic "matches a pattern -> solution" solvers, like 1st_linear, Bernoulli, and so on. For those it should be very clear what is common between all the solvers and what is currently being done redundantly that could be refactored into a superclass. Everything here is private API so it won't be a big deal to refactor things later if we determine they need to be more general.

[oscarbenjamin]

I picked algebraic just because it was the easiest to factor :)

Quite a few of the others just match a particular form of equation so there should be some way to do that efficiently so that other forms can be added easily in future. Working through all that is the difficult part though...

[Mohitbalwani26]

@oscarbenjamin can you please explain this line? why is it required?

[oscarbenjamin]

This line imports the things that are defined in single.py so that they can be used as part of dsolve which is in this module.

[Mohitbalwani26]

okay, Now i understood your idea properly. Thanks

[oscarbenjamin]

@Mohitbalwani26 and @V1krant this PR is now rebased onto master after merging #18414. I think that the best thing to do now is not to merge this yet but for either/both of you to try and build something off of this pull request. The way to do that is

$ git remote add oscarbenjamin https://github.com/oscarbenjamin/sympy.git
$ git fetch oscarbenjamin
$ git checkout oscarbenjamin/pr_dsolve_refactor
$ git checkout -b pr_dsolve_refactor   # Create your own branch
$ git push -u origin pr_dsolve_refactor

Then you have a branch called pr_dsolve_refactor which has the commits from here as the starting commits. You can commit and push your own changes and then open a pull request to sympy master so that we can see what happens when the tests are run and discuss the changes.

What we need to figure out next is a good way of organising these classes since many will be very similar. See the comment above #18403 (comment) from @asmeurer for a good suggestion. The idea is that many of the ODE solvers follow a particular pattern that they match a specific form of ODE and then return the general form of the solution based on that. Maybe there should be a superclass that can handle common functionality for that like:

class SinglePatternSolver(SingleODESolver):
    # Code that handles simplification, matching and expects
    # subclasses to define a .equation() method

class FirstLinear(SinglePatternSolver):
    def equation(func, x, n): # This is a function of n so it can work for the nth order solvers
        p, q = self.coeffs() # Returns the Wild coefficients
        return p*func + func.diff(x) - q

class Bernoulli(SinglePatternSolver):
    def equation(func, x, n):
        # return a different form

I think that we need to get this so that there are a few solvers looking good before merging anything.

[Mohitbalwani26]

@oscarbenjamin I am trying to refactor undetermined_coefficients. First of all the equation should be checked for nth_linear_match so should i make new class for nth_linear_match because for variation of parameters also this condition is necessary?

if I am making a class for undetermined_coefficients then in _get_general_solution there will be _solve_undetermined_coefficients right?

[Mohitbalwani26]

@oscarbenjam i have another question what about order of ode shouldn't we define in SingleODESolver because lot of methods require that?

[oscarbenjamin]

@oscarbenjam i have another question what about order of ode shouldn't we define in SingleODESolver because lot of methods require that?

I think that it would be better handled by SingleODEProblem rather than SingleODESolver.

[Mohitbalwani26]

@oscarbenjamin what should i implement next?

[oscarbenjamin]

You could try factoring out another solver.

[Mohitbalwani26]

@oscarbenjamin can you please suggest which should i go for i.e which is easier to implement now like nth_alegbraic?

[smitgajjar]

Hi @oscarbenjamin, I am new here. Can I contribute to the refactoring of another ode solver as mentioned on ideas page? Please guide me on how to proceed if possible.

[vikrantmalik051]

@oscarbenjamin , Could you please let me know what concept did you use here, because match is coming out to be of <class 'dict'> even when the ode and hint is for almost_linear or any other type which we have transformed to a class.

[oscarbenjamin]

The match variable here is what was stored in the matching_hints dict for this particular hint in classify_ode.

[oscarbenjamin]

Everyone is welcome to contribute. Note that at this stage we are just exploring ideas here. We need something more complete before any part of this should be merged. If you want to open a pull request based on this PR then that's fine. Once we are happy with it it can be merged into this PR.

[oscarbenjamin]

I've just pushed a commit that implements the pattern matching idea for the Bernoulli solver.

@asmeurer what do you think of it?

If we can do the same for the other solvers (FirstLinear and AlmostLinear) then this PR will just need tidying up and I think it can be good to merge.

Note that the changes have simplified the Bernoulli class so that it only has minimal code to specify the pattern of the ODE that it matches and the form of the general solution. Note also that using pattern matching has improved the matching code for this solver so that it now (correctly) matches more ODEs than it did before.

[oscarbenjamin]

@V1krant @mohitacecode the refactoring work will be a lot easier once this is merged. All this needs right now is to make the FirstLinear and AlmostLinear solvers use the same structure as the Bernoulli solver.

[Mohitbalwani26]

@V1krant @mohitacecode the refactoring work will be a lot easier once this is merged. All this needs right now is to make the FirstLinear and AlmostLinear solvers use the same structure as the Bernoulli solver.

currently AlmostLinear is inheriting FirstLinear so what is expected to have the structure for it?

[oscarbenjamin]

currently AlmostLinear is inheriting FirstLinear so what is expected to have the structure for it?

I don't think there's any need for one to inherit the other if they are implemented like Bernoulli.

[Mohitbalwani26]

if @mohitacecode and @V1krant are going for this then i will try another solver otherwise i would like to give it a try.

[oscarbenjamin]

Sorry @Mohitbalwani26 I meant to tag you there. Anyone is free to work on this.

[oscarbenjamin]

@asmeurer what do you think of this? Anyone else?

If this is roughly the direction that we want to go then I think it would be good to merge this and then improve it in subsequent PRs.

[Mohitbalwani26]

@oscarbenjamin Firstlinear should be inherited from SinglePatternODESolver or SingleODESolver?

[oscarbenjamin]

Firstlinear should be inherited from SinglePatternODESolver or SingleODESolver?

I think it should inherit SinglePatternODESolver.

[vikrantmalik051]

@oscarbenjamin, This method may require further modifications for cases like Riccati_special_minus2 where after matching, it is checked if some of the coefficients are zero.

sympy/sympy/solvers/ode/ode.py

Lines 1091 to 1099 in 35be3a0

	## Riccati special n == -2 case: a2*y'+b2*y**2+c2*y/x+d2/x**2 == 0
	r = collect(reduced_eq,
	f(x), exact=True).match(a2*df + b2*f(x)**2 + c2*f(x)/x + d2/x**2)
	if r and r[b2] != 0 and (r[c2] != 0 or r[d2] != 0):
	r['a2'] = a2
	r['b2'] = b2
	r['c2'] = c2
	r['d2'] = d2
	matching_hints["Riccati_special_minus2"] = r

Should we implement some other method for matching such ode or is there a workaround for this?

[oscarbenjamin]

Is it necessary for those to be non-zero or would the general solution apply equally well when those coefficients are zero?

[vikrantmalik051]

It is necessary for b i.e., r[b2] to be non-zero as the denominator term is 2*b*x in the solution. But I think this can be resolved by using
b = Wild('b', exclude=[x, f(x), f(x).diff(x), 0]) in _wilds method. The other problem is checking the or condition (in line 1094 in above code segment) for r[c2] and r[d2]. Here, if both of them are zero, then the solution is not real.

[oscarbenjamin]

We also need to bear in mind the possibility that the coefficient might be a symbol that is not known to be zero. We can generally handle that sort of thing using a Piecewise in sympy e.g.:

In [282]: x = Symbol('x')                                                                                                                      

In [283]: Piecewise((1/x, Ne(x, 0)), (0, True))                                                                                                
Out[283]: 
⎧1           
⎪─  for x ≠ 0
⎨x           
⎪            
⎩0  otherwise

In [284]: Piecewise((1/x, Ne(x, 0)), (0, True)).subs(x, 2)                                                                                     
Out[284]: 1/2

[sympy-bot]

🟠

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

• 346e9dd:
  • sympy/solvers/ode/single.py
  • sympy/solvers/ode/tests/test_single.py

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

[oscarbenjamin]

I've merged in a commit from @Mohitbalwani26 that rewrites the FirstLinear and AlmostLinear solvers using pattern-matching like the Bernoulli solver. I still think that the AlmostLinear matching code can be simplified significantly but this is good progress.

[oscarbenjamin]

At this point I would like to merge this so that future improvements to the idea can come as separate PRs.

Does anyone want to review this?

[Mohitbalwani26]

@oscarbenjamin should i start refactoring another solver if yes then which i should start? Or should i wait for this to get merged?

[oscarbenjamin]

Okay I'm going to merge this now. Further improvements can come later.

In particular:

1. The tests for these solvers should be moved to test_single.py.
2. AlmostLinear should be simplified so that it just uses a pattern.

Thanks @Mohitbalwani26 and @V1krant for your help here. Further work can be done in normal PRs.

[vikrantmalik051]

Thanks @Mohitbalwani26 and @V1krant for your help here. Further work can be done in normal PRs.

Thanks to you too for your guidance, @oscarbenjamin.

[Mohitbalwani26]

Thanks @Mohitbalwani26 and @V1krant for your help here. Further work can be done in normal PRs.

@oscarbenjamin ,Thanks to you too for valuable feedbacks and suggestions which really kept me going.