WIP: removing bits which should go into packages #5
Conversation
Mostly this is Algs but a few other things too.
| abstract AbstractHeatProblem <: DEProblem | ||
| "`PdeSolution`: Wrapper for the objects obtained from a solver" | ||
| abstract AbstractPDEProblem <: DEProblem | ||
| "`DSolution`: type to hold the objects obtained from a solver" |
There was a problem hiding this comment.
In this package we should only define the abstract types of broad categories of equations, I think. A heat-problem seems to specific.
| abstract DESolution | ||
| abstract AbstractODESolution <: DESolution | ||
| abstract AbstractSDESolution <: AbstractODESolution # Needed for plot recipes | ||
| abstract AbstractDDESolution <: AbstractODESolution # Needed for plot recipes | ||
| abstract AbstractFEMSolution <: DESolution | ||
| abstract AbstractSensitivitySolution | ||
| "`Mesh`: An abstract type which holds a (node,elem) pair and other information for a mesh" | ||
| abstract Mesh |
There was a problem hiding this comment.
A mesh seems like an implementation detail which should go elsewhere.
There was a problem hiding this comment.
Then there should be a FEMMeshes.jl for holding meshing functionality common to PDEs.
There was a problem hiding this comment.
Ok, let's leave it here. (At some later stage, a PDE related interface package might make sense.)
| abstract Tableau | ||
|
|
||
| "`ODERKTableau`: A Runge-Kutta Tableau for an ODE integrator" | ||
| abstract ODERKTableau <: Tableau | ||
|
|
||
| "`DEIntegrator`: A DifferentialEquations Integrator type, used to initiate a solver." | ||
| abstract DEIntegrator |
There was a problem hiding this comment.
Don't know what this does.
There was a problem hiding this comment.
Nothing anymore. Good find.
| abstract DEIntegrator | ||
|
|
||
| abstract ParameterizedFunction <: Function | ||
| abstract SensitivityFunction <: ParameterizedFunction |
There was a problem hiding this comment.
If we follow PR #2 then this is not needed.
There was a problem hiding this comment.
How does that PR handle Sensitivity functions?
There was a problem hiding this comment.
Nevermind, I see. SensitivityFunction didn't really need to subtype ParameterizedFunction after all.
| alg,extra_kwargs = default_algorithm(prob;kwargs...) | ||
| solve(prob,alg,args...;default_set=true,kwargs...,extra_kwargs...) | ||
| end | ||
| # function solve(prob::DEProblem,args...;default_set=false,kwargs...) |
There was a problem hiding this comment.
I'm not sure how to do a default solver best. I think it has to be done package-wise: the first package providing Algs which gets loaded can set a default solver. But not sure how to implement it.
There was a problem hiding this comment.
It's definitely not package-wise because the default solver for different cases is many times in different packages. For example, if you're trying to solve a stiff equation on Float64's, Sundials's CVODE_BDF and ODEInterface's radau tend to do best, while both of those package don't provide excellent methods for non-stiff equations, and don't work on odd number types.
Also, if it was package-wise it would clash.
There was a problem hiding this comment.
I don't think this decision belongs into this interface package. I think this pkg should just provide an interface to hook into the JuliaDiffEq universe but not make judgment calls which solver is best for what. That is maybe something for DifferentialEquations.jl to do.
How about a system like so: DiffEqBase provides a dictionary (or some other datastructure) where each package hooking into DiffEqBase puts its solvers. Then there is some rating of the solvers on how suitable they are for one type of equation and say tolerances. Then the default algo will be picked from that list based on best suitability. I'll make an example PR.
There was a problem hiding this comment.
The defaults code could just move to DifferentialEquations.jl if you think that's best.
There was a problem hiding this comment.
Package-based default-choosing algorithms could still be available by the user passing in the package-wide abstract type?
| @@ -1,158 +0,0 @@ | |||
| abstract AbstractODEAlgorithm | |||
There was a problem hiding this comment.
Having these here definitely breaks encapsulation. And there shouldn't be any problem defining them in their respective packages.
There was a problem hiding this comment.
Then how do you do defaults?
| @@ -1,11 +1,3 @@ | |||
| macro def(name, definition) | |||
There was a problem hiding this comment.
Not sure what this does.
| @@ -1,158 +0,0 @@ | |||
| abstract AbstractODEAlgorithm | |||
There was a problem hiding this comment.
Then how do you do defaults?
|
I'm implementing these changes by hand to make sure everything (that is used) finds the right home and tests pass locally before merging. |
|
Note that I will be pushing to finish the SDE stuff today as well so that way those tests don't get in the way. |
|
Sounds good. I don't have time today and tomorrow I will be on a long haul flight. So I will not give much feedback. |
|
It's okay. The SDE stuff will intentionally look almost exactly like the ODE stuff (taking into account your changes) except the SDE problems will take two functions and a For the noise process I think it might be easiest to throw the draft to master and make sure everything is all working, and if you have any comments from there handle that. For now it's far too small to be its own thing. |
|
This should all be done by 8f23efa . You might want to double check (it has the stochastic changes in there too. Again, nothing big except the noise process). I'll have to put in some minor tags all around for everything to reflect this (until then, tests fail almost everywhere because two package pretty much both export the same thing somewhere). |
This PR removes bits which I think should be in packages. In particular the all the algorithms needen't and shouldn't be defined here. If I understood correctly, this is my answer to SciML/Roadmap#9.
Note that this of course also necessitates changes in downstream pkgs.