[WIP] Adding pooled callbacks to VectorContinousCallback #523
Conversation
|
I am not convinced that it handles non root-finding callbacks correctly. There are currently no test on callbacks at all. |
|
https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/src/integrators/integrator_utils.jl#L251 this is where you are failing. You have to make it's type |
|
How come this isn't caught by the tests? |
|
Because they are behind the |
|
Ahh okay, so yeah it definitely needs a downstream test. |
|
Well test/callbacks.jl is quiet sparse in general. I am not sure if normal callbacks get tested appropriately by it. |
|
A lot should be covered by downstream tests such as https://github.com/SciML/DiffEqBase.jl/blob/master/test/downstream/event_detection_tests.jl. |
| @@ -209,14 +213,13 @@ function VectorContinuousCallback(condition,affect!,len; | |||
| affect_neg! = affect!, | |||
| interp_points=10, | |||
| dtrelax=1, | |||
| abstol=10eps(),reltol=0) | |||
| abstol=10eps(),reltol=0, pooltol=missing, pool_events=false) | |||
There was a problem hiding this comment.
IMO usually in DiffEq default values are set to nothing (as indicated by the docstring) or a sensible default, maybe one could use the same as abstol?
There was a problem hiding this comment.
This is very prototypie but thanks for your input, the doc strings aren't really updated ...
There was a problem hiding this comment.
The convention for the pooltol is also still in discussion, so i won't merge that proposal but keep in mind that doc strings would need to be added
| if callback.pool_events | ||
| tmp = get_condition(integrator, callback, integrator.dt + new_t) | ||
| if callback.pooltol isa Missing | ||
| # This is still dubious | ||
| pool_tol = eps(integrator.t + new_t) + eps(typeof(tmp[end])) | ||
| else | ||
| pool_tol = callback.pooltol | ||
| end | ||
| min_event_idx = findall(x-> abs(x) < pool_tol, tmp) | ||
| end |
There was a problem hiding this comment.
I'm a bit surprised that one has to redo all calculations here. Is it not possible to change lines such as
DiffEqBase.jl/src/callbacks.jl
Lines 735 to 738 in a13b48b
min_t and min_event_idx are not replaced but that one gets a list of min_t for every index, which then allows to select all indices within some tolerance later? And to change DiffEqBase.jl/src/callbacks.jl
Line 751 in a13b48b
DiffEqBase.jl/src/callbacks.jl
Line 755 in a13b48b
There was a problem hiding this comment.
Yes we discussed that, it is the initial PR, so lets get that perfect
There was a problem hiding this comment.
The code is not optimized since it does not work yet.
I have decided to keep the pooled callback logic out of branches for different types of time resolving, so there is just a single place to change it.
There was a problem hiding this comment.
I don't think this is necessarily the right direction. You're not looking for if the other values are near zero at the same time, you're looking for if there is another zero crossing nearby. One decent approximation would be to find the earliest event, and the move forward in time by some tolerance, and check the condition. You'd then take all of the events that were triggered in that time, and you'd know whether they are up or down crossings. Multiple crossings are extremely unlikely if this is on the size of a*eps(t). This would be insensitive to the relative size of the values.
This should show up in my repo only Updated my branch to keep up
I need help with this. I am currently running into:
ERROR (update): MethodError: Cannot convert an object of type Array{Int64,1} to an object of type Int64at
/.julia/packages/OrdinaryDiffEq/JrtsK/src/integrators/integrator_utils.jl:251Looks like
integrator.vector_event_last_timeexpects to be an Integer.