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

Extend WorkPrecisionSet to DDE problems #7

Merged
merged 2 commits into from
Apr 9, 2018
Merged

Extend WorkPrecisionSet to DDE problems #7

merged 2 commits into from
Apr 9, 2018

Conversation

devmotion
Copy link
Member

I just noticed that currently no DDE benchmarks can be performed since no WorkPrecisionSet can be created for DDE problems. It seems this issue is caused by the restructuring in 8de789c#diff-826047b6175ac9aca7e1958367322880.

@ChrisRackauckas
Copy link
Member

Would a better way of handling this be to make both it and AbstractDAEProblem subtypes of AbstractODEProblem? DAEs should get the same treatment here.

@coveralls
Copy link

coveralls commented Apr 9, 2018

Coverage Status

Coverage remained the same at 95.984% when pulling a79e9d7 on devmotion:fix_dde into 31dbb34 on JuliaDiffEq:master.

@codecov
Copy link

codecov bot commented Apr 9, 2018

Codecov Report

Merging #7 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #7   +/-   ##
=======================================
  Coverage   95.98%   95.98%           
=======================================
  Files           7        7           
  Lines        7172     7172           
=======================================
  Hits         6884     6884           
  Misses        288      288
Impacted Files Coverage Δ
src/benchmark.jl 83.68% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31dbb34...a79e9d7. Read the comment docs.

@devmotion
Copy link
Member Author

I like your idea. The only problem I see is that this type structure might not go well with SciML/DelayDiffEq.jl#29. Another alternative would be to introduce a new abstract supertype for all these problems, but I'm not completely sure about the (dis)advantages of this approach.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Apr 9, 2018

I am thinking that everything goes under AbstractODEProblem or AbstractRODEProblem, keeping stochastic separate from deterministic since they are treated very differently. OrdinaryDiffEq.jl should start handling DAEs soon enough while StochasticDiffEq.jl already patches in so that way it can handle both RODEs and SDEs, so we're already moving in the direction of clumping these things together. DelayDiffEq.jl should probably dispatch on that level to get the correct implementation when the time comes.

@devmotion
Copy link
Member Author

Yeah, separating stochastic and deterministic problems is the best approach, I guess. I'm just slightly confused how AbstractDDEProblem would fit in that type hierarchy, considering that it could be both stochastic and deterministic. Wouldn't this require two different subtypes such as AbstractDDEProblem and AbstractSDDEProblem? But maybe it's not necessary to discuss that now... Anyway, I guess I can close this issue since we should rather change the current type hierarchy?

@ChrisRackauckas
Copy link
Member

Just add AbstractDAEProblem to the deterministic dispatches here and we can merge. We can discuss better use of the type hierarchy in another issue.

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.

Merge when tests pass

@ChrisRackauckas ChrisRackauckas merged commit 099d03a into SciML:master Apr 9, 2018
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.

3 participants