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

Improve save_idxs #180

Merged
merged 3 commits into from
Aug 29, 2017
Merged

Improve save_idxs #180

merged 3 commits into from
Aug 29, 2017

Conversation

devmotion
Copy link
Member

When I wrote a notebook of benchmarks for DelayDiffEq, I discovered that save_idxs does not seem to work completely correct. First I thought this is a problem in DelayDiffEq (and there might still be issues there), but it is at least partly also a problem in OrdinaryDiffEq: Using save_idxs such that typeof(u[save_idxs]) != typeof(u) does not work. So e.g. it is not possible to extract single components of the state vector with scalars, and maybe even worse it is not possible to extract a vector or vectorized component of a matrix. When I had fixed this issue and finally could extract arbitrary components I noticed another issue: it is not possible to plot solutions retrieved by scalar indexing since the interpolation of exactly that case is broken. Hence this PR additionally includes an improved interpolation that covers this case. Use cases and examples are given in the added test file.

@codecov
Copy link

codecov bot commented Aug 29, 2017

Codecov Report

Merging #180 into master will increase coverage by 0.08%.
The diff coverage is 72.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   71.75%   71.83%   +0.08%     
==========================================
  Files          61       61              
  Lines       21165    21230      +65     
==========================================
+ Hits        15187    15251      +64     
- Misses       5978     5979       +1
Impacted Files Coverage Δ
src/integrators/general_rosenbrock_integrator.jl 0% <0%> (ø) ⬆️
src/integrators/linear_integrators.jl 0% <0%> (ø) ⬆️
src/integrators/iif_integrators.jl 0% <0%> (ø) ⬆️
src/integrators/exponential_rk_integrators.jl 0% <0%> (ø) ⬆️
src/integrators/generic_implicit_integrators.jl 95.4% <100%> (ø) ⬆️
src/integrators/ssprk_integrators.jl 97.32% <100%> (ø) ⬆️
src/integrators/threaded_rk_integrators.jl 100% <100%> (ø) ⬆️
src/integrators/verner_rk_integrators.jl 72.4% <100%> (-0.17%) ⬇️
src/integrators/rkn_integrators.jl 100% <100%> (ø) ⬆️
src/integrators/split_integrators.jl 100% <100%> (ø) ⬆️
... and 13 more

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 a63e788...3b760a6. Read the comment docs.

@coveralls
Copy link

coveralls commented Aug 29, 2017

Coverage Status

Coverage increased (+0.08%) to 71.837% when pulling ff85371 on devmotion:fix_saveidxs into a63e788 on JuliaDiffEq:master.

@coveralls
Copy link

coveralls commented Aug 29, 2017

Coverage Status

Coverage remained the same at ?% when pulling 3b760a6 on devmotion:fix_saveidxs into a63e788 on JuliaDiffEq:master.

@@ -107,8 +107,8 @@ function ode_interpolation(tvals,id,idxs,deriv)
tdir*tvals[idx[1]] < tdir*ts[1] && error("Solution interpolation cannot extrapolate before the first timepoint. Either start solving earlier or use the local extrapolation from the integrator interface.")
if typeof(idxs) <: Number
vals = Vector{eltype(first(timeseries))}(length(tvals))
elseif typeof(idxs) <: AbstractVector
vals = Vector{Vector{eltype(first(timeseries))}}(length(tvals))
elseif typeof(idxs) <: AbstractArray
Copy link
Member

Choose a reason for hiding this comment

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

...! Thanks. For some reason I couldn't find out how to do this before haha

@ChrisRackauckas
Copy link
Member

Thanks. I actually just couldn't find out how to do this before.

Would you mind making similar changes to StochasticDiffEq.jl? It should be incorrect in the same ways since the code is so similar.

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

3 participants