-
-
Couldn't load subscription status.
- Fork 233
Support for Inputs Feature #3998
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
base: master
Are you sure you want to change the base?
Conversation
Benchmark Results (Julia vlts)Time benchmarks
Memory benchmarks
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
src/systems/inputs.jl
Outdated
| using SymbolicIndexingInterface | ||
| using Setfield | ||
| using StaticArrays | ||
| using OrdinaryDiffEqCore |
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 top level
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.
fixed
| events::Tuple{SymbolicDiscreteCallback} | ||
| vars::Tuple{SymbolicUtils.BasicSymbolic{Real}} |
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 concrete
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.
fixed
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 fixed. Tuples are only concrete if you also choose a size for them
src/systems/inputs.jl
Outdated
| return sys | ||
| end | ||
|
|
||
| function DiffEqBase.solve(prob::SciMLBase.AbstractDEProblem, inputs::Vector{Input}, args...; kwargs...) |
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 violates the contract, and its not DiffEqBase
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.
fixed
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.
The added field isn't handled in namespacing. It looks like the intended behavior is for input_functions to only be populated on a simplified system? Consequently, I would recommend that build_input_functions should either be a model transformation in basic_transformations.jl or an optional simplification pass that can be included in additional_passes keyword of mtkcompile (similar to IfLifting).
| data::SVector | ||
| time::SVector |
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.
Why specifically SVector? Not only is this not concrete, I don't think it doesn't offer any performance gain given how it is used.
| n = length(data) | ||
| return Input(var, SVector{n}(data), SVector{n}(time)) |
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 is type-unstable. n isn't inferrable.
src/systems/inputs.jl
Outdated
| return Input(var, SVector{n}(data), SVector{n}(time)) | ||
| end | ||
|
|
||
| struct InputFunctions |
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.
Why do the fields of this struct need to be wrapped in Tuple{...}?
|
|
||
| struct InputFunctions | ||
| events::Tuple{SymbolicDiscreteCallback} | ||
| vars::Tuple{SymbolicUtils.BasicSymbolic{Real}} |
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.
Fairly minor, but vars is a bit of a misnomer for a field only containing one variable.
src/systems/inputs.jl
Outdated
| struct InputFunctions | ||
| events::Tuple{SymbolicDiscreteCallback} | ||
| vars::Tuple{SymbolicUtils.BasicSymbolic{Real}} | ||
| setters::Tuple{SymbolicIndexingInterface.ParameterHookWrapper} |
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.
SII.ParameterHookWrapper is not public API.
| return nothing | ||
| end | ||
| function set_input!(integrator, var, value::Real) | ||
| set_input!(get_input_functions(integrator.f.sys), integrator, var, value) |
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.
Accessing sys during a solve is discouraged. The solve should not involve symbolic quantities. This is a lot of unnecessary, type-unstable overhead. Solves should also work without the system - anything possible symbolically should be possible without the symbolic infrastructure.
|
|
||
| # Here we ensure the inputs have metadata marking the discrete variables as parameters. In some | ||
| # cases the inputs can be fed to this function before they are converted to parameters by mtkcompile. | ||
| vars = SymbolicUtils.BasicSymbolic[isparameter(x) ? x : toparam(x) |
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 think build_input_functions should be called as a model transformation after mtkcompile, and verify that the list of inputs it gets are inputs (and part of the list of parameters). This is more robust than trusting that the user passed values around correctly and trusting that there won't be bugs in the future.
| end | ||
| end | ||
|
|
||
| @set! sys.discrete_events = events |
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 would overwrite any pre-existing discrete events in the system.
|
|
||
| # ensure that the ODEProblem does not complain about missing parameter map | ||
| if !haskey(defaults, x) | ||
| push!(defaults, x => 0.0) |
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 is mutating the defaults stored in sys. Model transformations should ideally be out of place.
| end | ||
|
|
||
| function DiffEqBase.solve(prob::SciMLBase.AbstractDEProblem, inputs::Vector{Input}, args...; kwargs...) | ||
| tstops = Float64[] |
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.
It's not guaranteed that the time is Float64.
|
I thought about this a bit. In my opinion, the best way to phrase this is a model transformation in |
|
|
||
| ModelingToolkit supports setting input values during simulation for variables marked with the `[input=true]` metadata. This is useful for real-time simulations, hardware-in-the-loop testing, interactive simulations, or any scenario where input values need to be determined during integration rather than specified beforehand. | ||
|
|
||
| To use this functionality, variables must be marked as inputs using the `[input=true]` metadata and specified in the `inputs` keyword argument of `@mtkcompile`. |
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.
variables must be marked as inputs using the
[input=true]metadata
is this actually required? The inputs are specified
in the
inputskeyword argument of@mtkcompile
anyway. No other input-using functionality, like linearization or generation of input-dependent functions, rely on the input metadata
I really don't want to bake the input data into the |
|
That's not true. If you do it with FunctionWrappers or DataInterpolations you can just swap the interpolation via |
| return sys | ||
| end | ||
|
|
||
| function CommonSolve.solve(prob::SciMLBase.AbstractDEProblem, inputs::Vector{Input}, args...; kwargs...) |
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.
It still violates the dispatch contract that SciML does. The second argument should be the algorithm, if we do this. But I don't think it makes sense to do this as a solve, it violates a few other principles of solve
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 want to keep the data fully separate from the System and the ODEProblem. We need some way to be able to solve a problem where data is fed in at the top level. I have benchmarks that show that this is much more efficient, and follows the norm for what other modeling platforms offer. If we move inputs as the 3rd argument, will that be acceptable?
| for input::Input in inputs | ||
| tstops = union(tstops, input.time) | ||
| condition = (u, t, integrator) -> any(t .== input.time) | ||
| affect! = function (integrator) | ||
| @inbounds begin | ||
| i = findfirst(integrator.t .== input.time) | ||
| set_input!(integrator, input.var, input.data[i]) | ||
| end | ||
| end | ||
| push!(callbacks, DiscreteCallback(condition, affect!)) | ||
|
|
||
| # DiscreteCallback doesn't hit on t==0, workaround... | ||
| if input.time[1] == 0 | ||
| prob.ps[input.var] = input.data[1] | ||
| end | ||
| 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.
This shouldn't be done here. This should instead be done by registering discontinuities for the interpolation function if it is a discontinuous one.
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.
There is no interpolation here, this is explicitly a discrete input. The point is to not have interpolation.
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.
That is ConstantInterpolation
It's not faster, I have benchmarks that show this. |
|
No, you're not understanding... it's not faster you're just doing search reduction so you're not benchmarking the right thing. Ultimately this just isn't the way to implement it. We just need a specialization on ConstantInterpolation that lowers it to a callback form to remove search. That needs none of the other extra interface breaking machinery here. This form is just impossible to maintain and breaks a lot of contracts, and most of it isn't necessary (and it's half broken in many scenarios 😅). That would also fix the type instabilities that are rampant in here (which end up breaking a lot of precompilation improvements) |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Closes issue #3823