Skip to content
This repository has been archived by the owner on Sep 9, 2024. It is now read-only.

Added DOPRI78 tableau from DifferentialEquations.jl #101

Closed
wants to merge 1 commit into from

Conversation

mauro3
Copy link
Contributor

@mauro3 mauro3 commented Jun 10, 2016

Note that I had to increase the test tolerance tol for the new solver to pass. This is not due to an in-accurate solver but due to the dense output only being 3rd order. This made this test fail:
https://github.com/JuliaLang/ODE.jl/blob/a0ef97d6249e62a089370787850ae697e3d94962/test/runtests.jl#L60

Note that I had to increase the test tolerance `tol` for the new solver
to pass.  This is not due to an in-accurate solver but due to the dense
output only being 3rd order.
@coveralls
Copy link

coveralls commented Jun 10, 2016

Coverage Status

Coverage increased (+0.04%) to 93.691% when pulling 9345a71 on mauro3:m3/dopri78 into a0ef97d on JuliaLang:master.

@codecov-io
Copy link

codecov-io commented Jun 10, 2016

Current coverage is 93.69%

Merging #101 into master will increase coverage by 0.04%

@@             master       #101   diff @@
==========================================
  Files             2          2          
  Lines           315        317     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            295        297     +2   
  Misses           20         20          
  Partials          0          0          

Powered by Codecov. Last updated by a0ef97d...9345a71

@ChrisRackauckas
Copy link
Member

The order of the dense output would render the test mostly moot. It's probably best to test at a calculated point (like the endpoint) since that would have a much higher accuracy for most of the methods. Then the tolerance could be far reduced (or dependent on the method order)

@mauro3
Copy link
Contributor Author

mauro3 commented Sep 2, 2016

Yes, the tests need some love but that should be coming with #49. I think it would be fine to mere this now, or close. However, the tableau is not copied to #49 yet.

@ChrisRackauckas
Copy link
Member

Alright. I'll take that as a separate issue then, but the tests will need to be overhauled (I think some kind of more precise integration test than what's going on here would help). Is that handled in #49? If not, an issue should be opened discussing some ideas on how to improve the tests.

But this can't merge until the conflicts are resolved. I'm kinda surprised that this would conflict with something. Is it just the BS2/3 line spacing change that conflicts with the just merged #97?

@mauro3
Copy link
Contributor Author

mauro3 commented Sep 2, 2016

Ok, I'll update this. I'll see whether I got time today, afterwards I'm away from the computer for a 5 days.

@pwl
Copy link

pwl commented Sep 2, 2016

@ChrisRackauckas I encourage you to post an issue for #49, the new tests are pretty fresh and we any comments/improvements on them are welcome. Also, dense output and integrators are now completely separated in #49, which means you can test an integrator without invoking dense output.

@mauro3 mauro3 closed this May 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants