Skip to content

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

Merged
merged 13 commits into from
Jun 7, 2025
Merged

Conversation

sivasathyaseeelan
Copy link
Contributor

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.

Copy link
Member

@isaacsas isaacsas left a 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.

@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.

Copy link
Member

@isaacsas isaacsas left a 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.

sivasathyaseeelan and others added 5 commits June 4, 2025 22:28
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@sivasathyaseeelan
Copy link
Contributor Author

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.

I think we can do this when we make VR_Direct as default. So we can do a final refactor

@ChrisRackauckas
Copy link
Member

Looks like this comment may still need to be addressed? #488 (review)

@ChrisRackauckas
Copy link
Member

I think we can do this when we make VR_Direct as default. So we can do a final refactor

I see, that's fine. Let's merge to get benchmarks kicked off and include this in the bump that changes the default.

@ChrisRackauckas ChrisRackauckas merged commit ccbb392 into SciML:master Jun 7, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants