-
Notifications
You must be signed in to change notification settings - Fork 17
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
A new approach for the validated integration of ODEs #84
Conversation
The test for the |
Thanks for addressing this! |
9a5343b
to
c8aefa2
Compare
Some tests, which are not currently passing, should be fixed by #91; you may need to rebase once it is merged. |
Great, I'll wait for it. |
ae5124b
to
2c376bd
Compare
@UzielLinares Can you rebase to current master? This should run the tests using GitHub actions, and (perhaps) solve the problems spotted by travis. (GH actions is much faster than travis.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made some comments in some parts to smooth what i think are some rough corners.
picard(dx, x0, box) | ||
|
||
Computes the picard (integral) operator for the initial condition `x0`. | ||
`dx` must be the rhs of the differential equation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And box
...?
src/validatedODEs.jl
Outdated
@inbounds for i in eachindex(x) | ||
dom = sign_tstep > 0 ? 0 .. δt : δt .. 0 | ||
x0 = sign_tstep > 0 ? dom.lo : dom.hi | ||
Δ = zero(Interval{Float64}) | ||
xTM1[i] = TaylorModel1(deepcopy(x[i]), Δ, x0, dom) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! I would suggest to port this back to validated_integ
.
src/validatedODEs.jl
Outdated
function ε_inflation!(xTM1K, f!, dx, x0, params, t, box, dof; ε=1e-10, δ=1e-5, | ||
maxsteps=20, extrasteps=50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is, if I understand it correctly, where the actual validation is assessed, right? I do not think this is reflected by its name. My suggestion is to change it for one that reflects its importance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you add some docstrings explaining the validation idea or including the reference of Buenger's paper?
if nsteps == maxsteps | ||
@warn ("Maximum number of validate steps reached.") | ||
end | ||
|
||
return nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is useful, but maybe we should return a boolean which states if the validation succeeded or not, which may be used in the validated_integ2
function to either shrink the time step, or do something else.
Pull Request Test Coverage Report for Build 668815412Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Thanks for rebasing to master! There was a broken test (windows, 1.3) which i couldn't understand, so I rerun the tests and now they are passing. |
There are still some broken tests on nightly (1.7.0-DEV) |
There seem to be some conflicts (in |
Maybe duplicating the tests (old a new methods are tested) is the best... |
There is one test that seems to be stuck... |
The tests pass, but there is something preventing a test to complete. I'll simply go ahead and merge this to have this functionality in master. We can finish whatever is left later in a different PR. Thanks a lot @UzielLinares!!! |
Pull Request Test Coverage Report for Build 668837497Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
No description provided.