Skip to content
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

M3/example #3

Merged
merged 4 commits into from
Aug 8, 2017
Merged

M3/example #3

merged 4 commits into from
Aug 8, 2017

Conversation

mauro3
Copy link
Contributor

@mauro3 mauro3 commented Nov 15, 2016

A simple example on how to implement the interface. This is based on top of branch m3/fns (PR #2)

sol2 = solve(p, BwdEulerAlg, tsteps=0:dt:1)

using Plots
plot(sol.t, sol.u)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use the plot recipe? This way of plotting doesn't scale to vectors of vectors and is more verbose.


function solve{uType,tType,isinplace}(p::ODEProblem{uType,tType,isinplace},
Alg::Type{BwdEulerAlg};
tsteps=error("provide tsteps"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we agreed on tsteps instead of tstops? At least that's what I put in the docs and can change it.


ana = t -> exp(u)

function solve{uType,tType,isinplace}(p::ODEProblem{uType,tType,isinplace},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason to restrict solve to a concrete type. Have it dispatch on AbstractODEProblem{uType,tType,isinplace} so it'll work for whatever ODE problem it's given (including test problems).

using DiffEqBase

# make a algorithm type
abstract EulerAlgs <: OrdinaryDiffEqAlgorithm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the algorithm should subtype AbstractODEAlgorithm. See how each package has an abstract.

https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/master/src/algorithms/ode_algorithms.jl#L130

ff(t,u) = u
ff(::Val{:jac}, t, u) = 1
p = ODEProblem(ff, 1.0, (0.0,1.0))
sol2 = solve(p, BwdEulerAlg, tsteps=0:dt:1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the example should show ODETestProblem, plot(sol,plotanalytic=true), and test_convergence. For the last:

dts = 1./2.^(8:-1:4)
sim = test_convergence(dts,prob,BwdEulerAlg)
plot(sim)
sim.𝒪est[:final]

on a test problem (linear ODE should be fine). Check out OrdinaryDiffEq's tests + DiffEqProblemLibrary if you need help. If this is difficult, let me know and I'll improve the devdocs (though most of it should already be there? Just at a high level without an example).

@ChrisRackauckas
Copy link
Member

This PR also has the function interface specs: accident?

@mauro3
Copy link
Contributor Author

mauro3 commented Nov 15, 2016

I did base it on top of m3/fns because I used has_jac. But I can change this.

@ChrisRackauckas
Copy link
Member

I did base it on top of m3/fns because I used has_jac

Makes sense.



using Plots
plot(sol, plot_analytic=true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gives me an error:

julia> include("euler-algs.jl")
ERROR: LoadError: type ODETestSolution has no field timeseries_analytic
 in macro expansion at /home/mauro/.julia/v0.5/DiffEqBase/src/solutions/solution_interface.jl:62 [inlined]

What do I need to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my bad. That's a bug in the plot recipe since I used to call the outputs timeseries and timseries_analytic (but liked just using u / y like PR49). It should work just by changing every timseries_analytic to u_analytic in the plot recipe. I can do that right now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed on master now. I wish there was a good way to include the plot recipes in tests without having to make Travis install a whole plotting package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I get:

ERROR: LoadError: BoundsError
 in getindex(::Float64, ::Int64) at ./number.jl:21
 in macro expansion at /home/mauro/.julia/v0.5/DiffEqBase/src/solutions/solution_interface.jl:84 [inlined]
 in apply_recipe(::Dict{Symbol,Any}, ::DiffEqBase.ODETestSolution{Array{Float64,1},Array{Float64,1},Float64,FloatRange{Float64},Array{Any,1},DiffEqBase.ODETestProblem{Float64,Float64,false,##26#28},FwdEulerAlg}) at /home/mauro/.julia/v0.5/RecipesBase/src/RecipesBase.jl:238
 in _process_userrecipes(::Plots.Plot{Plots.PyPlotBackend}, ::Dict{Symbol,Any}, ::Tuple{DiffEqBase.ODETestSolution{Array{Float64,1},Array{Float64,1},Float64,FloatRange{Float64},Array{Any,1},DiffEqBase.ODETestProblem{Float64,Float64,false,##26#28},FwdEulerAlg}}) at /home/mauro/.julia/v0.5/Plots/src/pipeline.jl:73
 in _plot!(::Plots.Plot{Plots.PyPlotBackend}, ::Dict{Symbol,Any}, ::Tuple{DiffEqBase.ODETestSolution{Array{Float64,1},Array{Float64,1},Float64,FloatRange{Float64},Array{Any,1},DiffEqBase.ODETestProblem{Float64,Float64,false,##26#28},FwdEulerAlg}}) at /home/mauro/.julia/v0.5/Plots/src/plot.jl:173
 in (::Plots.#kw##plot)(::Array{Any,1}, ::Plots.#plot, ::DiffEqBase.ODETestSolution{Array{Float64,1},Array{Float64,1},Float64,FloatRange{Float64},Array{Any,1},DiffEqBase.ODETestProblem{Float64,Float64,false,##26#28},FwdEulerAlg}) at ./<missing>:0
 in include_from_node1(::String) at ./loading.jl:488
while loading /home/mauro/.julia/v0.5/DiffEqBase/examples/euler-algs.jl, in expression starting on line 89

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try it now. I just tried it out and plot_analytic it should be all compatible with the interface with dense plotting but true and false.

@mauro3
Copy link
Contributor Author

mauro3 commented Nov 15, 2016

Also when running at the end of the script:

using DiffEqDevTools
dts = 1./2.^(8:-1:4)
sim = test_convergence(dts,p2,BwdEulerAlg)
plot(sim)
sim.𝒪est[:final]

(using master of DiffEqDevTools) I get:

julia> sim = test_convergence(dts,p2,BwdEulerAlg)
ERROR: MethodError: Cannot `convert` an object of type Type{BwdEulerAlg} to an object of type Array{Float64,1}
This may have arisen from a call to the constructor Array{Float64,1}(...),
since type constructors fall back to convert methods.
 in #solve#51(::Float64, ::Bool, ::Int64, ::DiffEqBase.ExplicitRKTableau, ::Bool, ::Void, ::Symbol, ::Bool, ::Bool, ::Array{Float64,1}, ::Array{Float64,1}, ::Bool, ::Float64, ::Rational{Int64}, ::Rational{Int64}, ::Void, ::Void, ::Rational{Int64}, ::Bool, ::Void, ::Void, ::Int64, ::Float64, ::Float64, ::Bool, ::OrdinaryDiffEq.#ODE_DEFAULT_NORM, ::OrdinaryDiffEq.#ODE_DEFAULT_ISOUTOFDOMAIN, ::Bool, ::Int64, ::String, ::Void, ::Array{Any,1}, ::DiffEqBase.#solve, ::DiffEqBase.ODETestProblem{Float64,Float64,true,#ff}, ::Type{DiffEqBase.Tsit5}, ::Type{T}, ::Array{Any,1}, ::Array{Any,1}) at /home/mauro/.julia/v0.5/OrdinaryDiffEq/src/solve.jl:155
 in (::DiffEqBase.#kw##solve)(::Array{Any,1}, ::DiffEqBase.#solve, ::DiffEqBase.ODETestProblem{Float64,Float64,true,#ff}, ::Type{DiffEqBase.Tsit5}, ::DataType, ::Array{Any,1}, ::Array{Any,1}) at ./<missing>:0 (repeats 2 times)
 in #solve#99(::Bool, ::Array{Any,1}, ::Function, ::DiffEqBase.ODETestProblem{Float64,Float64,true,#ff}, ::Type{T}, ::Vararg{Type{T},N}) at /home/mauro/.julia/v0.5/DiffEqBase/src/DiffEqBase.jl:62
 in (::DiffEqBase.#kw##solve)(::Array{Any,1}, ::DiffEqBase.#solve, ::DiffEqBase.ODETestProblem{Float64,Float64,true,#ff}, ::Type{T}) at ./<missing>:0
 in (::DiffEqDevTools.##20#21{Bool,Bool,Array{Any,1},Array{Float64,1},DiffEqBase.ODETestProblem{Float64,Float64,true,#ff},DataType})(::Int64) at ./<missing>:0
 in collect(::Base.Generator{UnitRange{Int64},DiffEqDevTools.##20#21{Bool,Bool,Array{Any,1},Array{Float64,1},DiffEqBase.ODETestProblem{Float64,Float64,true,#ff},DataType}}) at ./array.jl:307
 in #test_convergence#19(::Bool, ::Bool, ::Array{Any,1}, ::Function, ::Array{Float64,1}, ::DiffEqBase.ODETestProblem{Float64,Float64,true,#ff}, ::Type{T}) at /home/mauro/.julia/v0.5/DiffEqDevTools/src/convergence.jl:41
 in test_convergence(::Array{Float64,1}, ::DiffEqBase.ODETestProblem{Float64,Float64,true,#ff}, ::Type{T}) at /home/mauro/.julia/v0.5/DiffEqDevTools/src/convergence.jl:40

This seems to call into OrdinaryDiffEq, which probably shouldn't happen.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Nov 15, 2016

This seems to call into OrdinaryDiffEq, which probably shouldn't happen.

That's because you subtyped OrdinaryDiffEqAlgorithm, and your specific algorithm doesn't exist, so it called my solve command. You just ran into this: SciML/Roadmap#9

In fact, this is exactly the example I gave. The solution that I know of is you have to import this algorithm in DiffEqDevTools, otherwise it won't know about your solve when it calls solve. That's the problem that I don't know how to solve: all of the higher level analysis tools (parameter estimation, sensitivity analysis, dev tools) will have to depend on each solver it's compatible with... is that okay? (but if you take the same convergence analysis code and copy paste it in the REPL, it's fine)

That fact might solve itself with extern modules.

@mauro3
Copy link
Contributor Author

mauro3 commented Nov 15, 2016

Ah, maybe now I'll be able to follow that issue! I'll see again. Edit: see #5

Edit: but this also happens now that I subtype DiffEqBase.AbstractODEAlgorithm

immutable BwdEulerAlg <: EulerAlgs
end

function DiffEqBase.solve{uType,tType,isinplace}(p::AbstractODEProblem{uType,tType,isinplace},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't extend DiffEqBase.solve but created a new function, because solve was not exported (#4) it didn't warn me either. Thus, below in the call to test_convergence another solve was called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, the whole problem there was that solve wasn't exported? For some reason I thought it was...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The not exported meant that there was no warning, it would have worked otherwise.

Copy link
Member

@ChrisRackauckas ChrisRackauckas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine but #2 goes first.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Nov 16, 2016

Actually, shouldn't the example just go in devdocs? Seems weird to just put a random example here. Also that would give more room for explanation, and a tutorial would really help make it clear.

@mauro3
Copy link
Contributor Author

mauro3 commented Nov 16, 2016

DevDocs would be fine too. I just wanted to test-drive the interface and had to put it somewhere for discussion. So maybe leave it in this PR until finished discussing and then move to the docs?

@ChrisRackauckas
Copy link
Member

Sounds good

@ChrisRackauckas
Copy link
Member

Would you mind turning this code into a ExampleSolverDiffEq.jl? I think having a simple (unregistered) repo for the devdocs to point to that is a working example for how to do this is a good idea. I'd swap this code over myself but I'm not sure how to do that and still give you the credit.

@ChrisRackauckas ChrisRackauckas merged commit 457a1ad into master Aug 8, 2017
@mauro3 mauro3 deleted the m3/example branch August 8, 2017 19:36
@mauro3
Copy link
Contributor Author

mauro3 commented Aug 8, 2017

Ha, this was still valid! Good.

@ChrisRackauckas
Copy link
Member

Yeah, as much as it seems like code changes around here often, the core actually hasn't changed much at all in a year. I guess this was a good proof of that haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants