-
-
Notifications
You must be signed in to change notification settings - Fork 37
VR_DirectFW aggregator #488
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
Conversation
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.
Take a look at this code.
JumpProcesses.jl/src/aggregators/ssajump.jl
Lines 37 to 88 in 069e129
@inline function makewrapper(::Type{T}, aff) where {T} | |
# rewrap existing wrappers | |
if aff isa FunctionWrappers.FunctionWrapper | |
T(aff.obj[]) | |
elseif aff isa Function | |
T(aff) | |
else | |
error("Invalid type of affect function, $(typeof(aff)), expected a Function or FunctionWrapper.") | |
end | |
end | |
@inline function concretize_affects!(p::AbstractSSAJumpAggregator, | |
::I) where {I <: DiffEqBase.DEIntegrator} | |
if (p.affects! isa Vector) && | |
!(p.affects! isa Vector{FunctionWrappers.FunctionWrapper{Nothing, Tuple{I}}}) | |
AffectWrapper = FunctionWrappers.FunctionWrapper{Nothing, Tuple{I}} | |
p.affects! = AffectWrapper[makewrapper(AffectWrapper, aff) for aff in p.affects!] | |
end | |
nothing | |
end | |
@inline function concretize_affects!(p::AbstractSSAJumpAggregator{T, S, F1, F2}, | |
::I) where {T, S, F1, F2 <: Tuple, I <: DiffEqBase.DEIntegrator} | |
nothing | |
end | |
# setting up a new simulation | |
function (p::AbstractSSAJumpAggregator)(dj, u, t, integrator) # initialize | |
concretize_affects!(p, integrator) | |
initialize!(p, integrator, u, integrator.p, t) | |
register_next_jump_time!(integrator, p, integrator.t) | |
u_modified!(integrator, false) | |
nothing | |
end | |
# condition for jump to occur | |
@inline function (p::AbstractSSAJumpAggregator)(u, t, integrator) | |
p.next_jump_time == t | |
end | |
# executing jump at the next jump time | |
function (p::AbstractSSAJumpAggregator)(integrator::I) where {I <: DiffEqBase.DEIntegrator} | |
affects! = p.affects! | |
if affects! isa Vector{FunctionWrappers.FunctionWrapper{Nothing, Tuple{I}}} | |
execute_jumps!(p, integrator, integrator.u, integrator.p, integrator.t, affects!) | |
else | |
error("Error, invalid affects! type. Expected a vector of function wrappers and got $(typeof(affects!))") | |
end | |
generate_jumps!(p, integrator, integrator.u, integrator.p, integrator.t) | |
register_next_jump_time!(integrator, p, integrator.t) | |
nothing | |
end |
During callback initialization you need to concretize the affect as its type won't actually be known until the integrator has been created (which is after creation of the cache). Then when you need to use the affect you need to wrap the call to execute_affect!
in an outer function that does a manual type check like:
if affects! isa Vector{FunctionWrappers.FunctionWrapper{Nothing, Tuple{I}}} |
to ensure the compiler knows the type of the affect array.
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 suggest you just add a
const VR_JUMP_AGGREGATORS = (VR_FRM(), VR_Direct(), VR_DirectFW())
global constant where the types are defined. Then in the tests you can just loop over the elements of this instead of having to manually type out the tuple.
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
I think we can do this when we make VR_Direct as default. So we can do a final refactor |
Looks like this comment may still need to be addressed? #488 (review) |
I see, that's fine. Let's merge to get benchmarks kicked off and include this in the bump that changes the default. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.