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

CompositeAlgorithm and switching compatibility #64

Closed
ChrisRackauckas opened this issue Apr 9, 2018 · 9 comments
Closed

CompositeAlgorithm and switching compatibility #64

ChrisRackauckas opened this issue Apr 9, 2018 · 9 comments
Assignees

Comments

@ChrisRackauckas
Copy link
Member

This library is almost compatible with the CompositeAlgorithm. The issue is just the return. The solution of the integrator.integrator is appropriately the CompositeSolution, but the integrator's solution is just an ODESolution. The former is required because it has the alg_choice field which makes the interpolant match the chosen algorithm.

@ChrisRackauckas ChrisRackauckas changed the title CompositeAlgorithm compatibility CompositeAlgorithm and switching compatibility Apr 9, 2018
@devmotion devmotion self-assigned this Apr 11, 2018
@devmotion
Copy link
Member

I updated the type of the returned solution and added some tests in https://github.com/JuliaDiffEq/DelayDiffEq.jl/tree/composite. However, something seems to be incorrect/broken as the following example of the first Robertson problem in #67 shows:

sol = solve(prob_dde_rober1, MethodOfSteps(AutoTsit5(Rosenbrock23())); initial_order = 1)

all(sol.alg_choice .== 1)

plot(sol)

newplot

@ChrisRackauckas
Copy link
Member Author

If you check the solution, at the end the current algorithm should be 2, but all(sol.alg_choice .== 1) is true, so the interpolant is broken since it's using the wrong interpolation.

@ChrisRackauckas
Copy link
Member Author

integrator_utils.jl on line 88 has:

      if typeof(integrator.alg) <: OrdinaryDiffEqCompositeAlgorithm
        copyat_or_push!(integrator.sol.alg_choice,integrator.saveiter,integrator.cache.current)
      end

I don't think we update cache.current in the other cache.

@ChrisRackauckas
Copy link
Member Author

rober

@ChrisRackauckas
Copy link
Member Author

I merged master into your branch and fixed the current in the cache and now it's interpolating properly.

@ChrisRackauckas
Copy link
Member Author

@YingboMa letting you know this is working

@ChrisRackauckas
Copy link
Member Author

Is the branch missing anything or is it ready to merge? LGTM?

@devmotion
Copy link
Member

No, I guess it's complete and ready to merge. Thanks for the fix!

@ChrisRackauckas
Copy link
Member Author

Thank you too. Good team effort 👍

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

No branches or pull requests

2 participants