Enable use of keywords save_idxs, saveat, save_start, save_everystep, and dense #19

devmotion · 2017-07-24T12:52:46Z

This PR tries to solve the same issues as #18. Parameter estimation would be simplified when these keywords can be applied. Currently, the use of save_idxs and save_start is (usually) not possible, and the use of saveat, save_everystep, and dense leads to incorrect solutions. In contrast to the old PR this implementation does not use a second, separate solution with specified indices and time points immediately, but only after all calculations are done.

Moreover, if the final solution is not dense and does only contain time points specified by saveat, the ODE solution is already reduced during the calculations, i.e. time points that are not required for accurate interpolation of the time points specified by saveat are removed as soon as they are not needed anymore to correctly calculate the next step.

Some simple benchmarks comparing current master, the old PR, and this PR are available at https://gist.github.com/devmotion/97a1c30df6fe7e0774b6456cc0240a24.

coveralls · 2017-07-24T13:14:11Z

Coverage increased (+3.6%) to 81.609% when pulling 4b94aac on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

devmotion · 2017-07-24T13:22:26Z

I do not know why but apparently the build on Windows 32bit again fails because of some error bounds, (see #17 (comment)):

https://ci.appveyor.com/project/ChrisRackauckas/delaydiffeq-jl/build/1.0.85/job/i2n6fykov52cueo0

I did not even change those bounds... Tests pass on all other architectures.

coveralls · 2017-07-25T10:12:22Z

Coverage increased (+3.6%) to 81.609% when pulling 694b927 on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

ChrisRackauckas · 2017-07-28T17:48:40Z

src/solve.jl

-    # calculate and add errors to solution if analytic solution to problem exists
-    # can not use same mechanism as for ODE problems since analytic solutions for DDE depend
-    # on initial value u0
+    if integrator.opts.save_everystep && isempty(integrator.opts.saveat)


Can this be pulled out to a function call on the integrator? It would make solve a bit more legible.

ChrisRackauckas · 2017-07-28T17:51:01Z

src/solve.jl

+        errors = nothing
+    end
+
+    if integrator.opts.dense


Pull this out to a function too?

ChrisRackauckas · 2017-07-28T17:56:03Z

src/utils.jl

+
+            if !needed
+                # delete not required time point, function value, and interpolation data
+                deleteat!(integrator.integrator.sol.t, integrator.saveiter + 1)


won't deleateat! be slow to do one-by-one, and it would be much faster to just do the change at the end? I am not sure why this is needed if you're also doing the other thing at the end.

It's not necessary to delete these entries, but I wanted to check if there is any benefit in deleting unneeded entries already during the algorithm.
The problem is that the entries that can be deleted do not have to be successive entries - some entries might be needed to calculate interpolated values of the output solution whereas others can be removed completely... Maybe one could completely remove these entries and only keep the interpolated values - but I do not know if this could cause other problems....

I'd run timings on it. I know Julia amortizes push!, but I think it might re-allocate with eery deleteat! and subsequently ignore the amortization, making it quite expensive to delateat! very often. I would only want to use this code path then in cases where I have a really tight RAM limit that I am hitting. It would probably be good to keep it as an option.

coveralls · 2017-07-29T03:57:25Z

Coverage increased (+3.6%) to 81.609% when pulling 694b927 on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

ChrisRackauckas · 2017-07-29T04:11:39Z

Cool, that was it. Tests pass. So after moving those pieces out to a function and making an option for the deleteat! we can merge.

coveralls · 2017-07-29T07:27:13Z

Coverage increased (+4.04%) to 82.09% when pulling 651777e on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

coveralls · 2017-07-29T07:27:13Z

Coverage increased (+4.04%) to 82.09% when pulling 651777e on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

devmotion · 2017-07-29T07:29:34Z

Currently HDF5 has build errors on Windows 32bit:

INFO: Downloading: hdf5
WARNING: Unknown download failure, error code: 2148270086
WARNING: Retry 1/5 downloading: https://cache.julialang.org/http://download.opensuse.org/repositories/windows:/mingw:/win32/openSUSE_Leap_42.2/noarch/mingw32-hdf5-1.8.13-4.13.noarch.rpm
WARNING: Unknown download failure, error code: 2148270086
WARNING: Retry 2/5 downloading: https://cache.julialang.org/http://download.opensuse.org/repositories/windows:/mingw:/win32/openSUSE_Leap_42.2/noarch/mingw32-hdf5-1.8.13-4.13.noarch.rpm
WARNING: Unknown download failure, error code: 2148270086
WARNING: Retry 3/5 downloading: https://cache.julialang.org/http://download.opensuse.org/repositories/windows:/mingw:/win32/openSUSE_Leap_42.2/noarch/mingw32-hdf5-1.8.13-4.13.noarch.rpm
WARNING: Unknown download failure, error code: 2148270086
WARNING: Retry 4/5 downloading: https://cache.julialang.org/http://download.opensuse.org/repositories/windows:/mingw:/win32/openSUSE_Leap_42.2/noarch/mingw32-hdf5-1.8.13-4.13.noarch.rpm
WARNING: Unknown download failure, error code: 2148270086
WARNING: Retry 5/5 downloading: https://cache.julialang.org/http://download.opensuse.org/repositories/windows:/mingw:/win32/openSUSE_Leap_42.2/noarch/mingw32-hdf5-1.8.13-4.13.noarch.rpm
INFO: try running WinRPM.update() and retrying the install
================================[ ERROR: HDF5 ]=================================
LoadError: failed to download hdf5 0 from https://cache.julialang.org/http://download.opensuse.org/repositories/windows:/mingw:/win32/openSUSE_Leap_42.2/noarch/mingw32-hdf5-1.8.13-4.13.noarch.rpm.
while loading C:\Users\appveyor\.julia\v0.6\HDF5\deps\build.jl, in expression starting on line 39

I noticed the same errors some days ago when I tried to debug the Windows 32bit error but it was gone after some hours. So probably we can just try to restart the tests til it works...

coveralls · 2017-07-29T07:39:45Z

Coverage increased (+4.04%) to 82.09% when pulling 651777e on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

coveralls · 2017-07-29T07:43:01Z

Coverage increased (+4.04%) to 82.09% when pulling 651777e on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

devmotion · 2017-07-29T08:03:55Z

At least Travis CI builds pass but there's still the download error on Windows 32bit...

ChrisRackauckas · 2017-07-29T12:34:52Z

well since it did run Win32 once and pass I'm pretty confident it's fixed. Since all of the changes are made I'll merge now. Of course, this will be re-run in the future, so if the test fails when the caching gets fixed we can open it up again.

codecov-io · 2017-07-29T12:40:53Z

Codecov Report

Merging #19 into master will increase coverage by 4.04%.
The diff coverage is 87.61%.

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   78.04%   82.08%   +4.04%     
==========================================
  Files           8        9       +1     
  Lines         164      268     +104     
==========================================
+ Hits          128      220      +92     
- Misses         36       48      +12

Impacted Files	Coverage Δ
src/integrator_type.jl	`66.66% <ø> (ø)`	⬆️
src/integrator_interface.jl	`62.33% <100%> (+2.59%)`	⬆️
src/integrator_utils.jl	`87.35% <87.35%> (ø)`
src/solve.jl	`92.4% <87.5%> (-2.76%)`	⬇️

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 1ca6931...651777e. Read the comment docs.

coveralls · 2017-07-29T12:40:59Z

Coverage increased (+4.04%) to 82.09% when pulling 651777e on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

coveralls · 2017-07-29T12:40:59Z

Coverage increased (+4.04%) to 82.09% when pulling 651777e on devmotion:final_solution into 1ca6931 on JuliaDiffEq:master.

devmotion added 5 commits July 16, 2017 02:34

First implementation of output solution that is created after all steps

e5e1745

Minimize new allocations

d51d1d3

Add not dense solution and move fields to separate DEOption

15a8315

Save memory

e603957

Reduce solution of ODE integrator as soon as possible

4b94aac

devmotion mentioned this pull request Jul 24, 2017

Add support for saveat and save_idxs to MethodOfSteps algorithm #18

Closed

Improve calculation of analytical errors

694b927

devmotion mentioned this pull request Jul 25, 2017

Test errors on Windows 32bit #20

Closed

ChrisRackauckas reviewed Jul 28, 2017

View reviewed changes

src/solve.jl Outdated

errors = nothing

end

if integrator.opts.dense

Copy link

Member

ChrisRackauckas Jul 28, 2017

Choose a reason for hiding this comment

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

Pull this out to a function too?

ChrisRackauckas reviewed Jul 28, 2017

View reviewed changes

Add option to disable reduction of ODE solution

651777e

ChrisRackauckas closed this Jul 29, 2017

ChrisRackauckas reopened this Jul 29, 2017

devmotion closed this Jul 29, 2017

devmotion reopened this Jul 29, 2017

devmotion closed this Jul 29, 2017

devmotion reopened this Jul 29, 2017

devmotion closed this Jul 29, 2017

devmotion reopened this Jul 29, 2017

ChrisRackauckas approved these changes Jul 29, 2017

View reviewed changes

ChrisRackauckas merged commit 9223d96 into SciML:master Jul 29, 2017

devmotion deleted the final_solution branch July 29, 2017 12:43

ChrisRackauckas mentioned this pull request Aug 4, 2017

Optimizing DDEs SciML/DiffEqParamEstim.jl#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable use of keywords save_idxs, saveat, save_start, save_everystep, and dense #19

Enable use of keywords save_idxs, saveat, save_start, save_everystep, and dense #19

devmotion commented Jul 24, 2017

coveralls commented Jul 24, 2017 •

edited

devmotion commented Jul 24, 2017

coveralls commented Jul 25, 2017 •

edited

ChrisRackauckas Jul 28, 2017

ChrisRackauckas Jul 28, 2017

ChrisRackauckas Jul 28, 2017

devmotion Jul 28, 2017

ChrisRackauckas Jul 28, 2017

coveralls commented Jul 29, 2017

ChrisRackauckas commented Jul 29, 2017

coveralls commented Jul 29, 2017

coveralls commented Jul 29, 2017

devmotion commented Jul 29, 2017

coveralls commented Jul 29, 2017 •

edited

coveralls commented Jul 29, 2017 •

edited

devmotion commented Jul 29, 2017

ChrisRackauckas commented Jul 29, 2017

codecov-io commented Jul 29, 2017 •

edited

coveralls commented Jul 29, 2017

coveralls commented Jul 29, 2017

Enable use of keywords save_idxs, saveat, save_start, save_everystep, and dense #19

Enable use of keywords save_idxs, saveat, save_start, save_everystep, and dense #19

Conversation

devmotion commented Jul 24, 2017

coveralls commented Jul 24, 2017 • edited

devmotion commented Jul 24, 2017

coveralls commented Jul 25, 2017 • edited

ChrisRackauckas Jul 28, 2017

Choose a reason for hiding this comment

ChrisRackauckas Jul 28, 2017

Choose a reason for hiding this comment

ChrisRackauckas Jul 28, 2017

Choose a reason for hiding this comment

devmotion Jul 28, 2017

Choose a reason for hiding this comment

ChrisRackauckas Jul 28, 2017

Choose a reason for hiding this comment

coveralls commented Jul 29, 2017

ChrisRackauckas commented Jul 29, 2017

coveralls commented Jul 29, 2017

coveralls commented Jul 29, 2017

devmotion commented Jul 29, 2017

coveralls commented Jul 29, 2017 • edited

coveralls commented Jul 29, 2017 • edited

devmotion commented Jul 29, 2017

ChrisRackauckas commented Jul 29, 2017

codecov-io commented Jul 29, 2017 • edited

Codecov Report

coveralls commented Jul 29, 2017

coveralls commented Jul 29, 2017

coveralls commented Jul 24, 2017 •

edited

coveralls commented Jul 25, 2017 •

edited

coveralls commented Jul 29, 2017 •

edited

coveralls commented Jul 29, 2017 •

edited

codecov-io commented Jul 29, 2017 •

edited