Skip to content

refactor: implement new additions to SII interface #2544

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 4 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ SimpleNonlinearSolve = "0.1.0, 1"
SparseArrays = "1"
SpecialFunctions = "0.7, 0.8, 0.9, 0.10, 1.0, 2"
StaticArrays = "0.10, 0.11, 0.12, 1.0"
SymbolicIndexingInterface = "0.3.1"
SymbolicIndexingInterface = "0.3.11"
SymbolicUtils = "1.0"
Symbolics = "5.24"
URIs = "1"
Expand Down
22 changes: 11 additions & 11 deletions docs/src/basics/FAQ.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,17 @@ Indexes into the `MTKParameters` object take the form of `ParameterIndex` object
are similarly undocumented. Following is the list of behaviors that should be relied on for
`MTKParameters`:

- It implements the SciMLStructures interface.
- It can be queried for parameters using functions returned from
`SymbolicIndexingInterface.getp`.
- `getindex(::MTKParameters, ::ParameterIndex)` can be used to obtain the value of a
parameter with the given index.
- `setindex!(::MTKParameters, value, ::ParameterIndex)` can be used to set the value of a
parameter with the given index.
- `parameter_values(sys, sym)` will return a `ParameterIndex` object if `sys` has been
`complete`d (through `structural_simplify`, `complete` or `@mtkbuild`).
- `copy(::MTKParameters)` is defined and duplicates the parameter object, including the
memory used by the underlying buffers.
- It implements the SciMLStructures interface.
- It can be queried for parameters using functions returned from
`SymbolicIndexingInterface.getp`.
- `getindex(::MTKParameters, ::ParameterIndex)` can be used to obtain the value of a
parameter with the given index.
- `setindex!(::MTKParameters, value, ::ParameterIndex)` can be used to set the value of a
parameter with the given index.
- `parameter_values(sys, sym)` will return a `ParameterIndex` object if `sys` has been
`complete`d (through `structural_simplify`, `complete` or `@mtkbuild`).
- `copy(::MTKParameters)` is defined and duplicates the parameter object, including the
memory used by the underlying buffers.

Any other behavior of `MTKParameters` (other `getindex`/`setindex!` methods, etc.) is an
undocumented internal and should not be relied upon.
Expand Down
2 changes: 2 additions & 0 deletions src/systems/abstractsystem.jl
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,8 @@ function SymbolicIndexingInterface.is_observed(sys::AbstractSystem, sym)
!is_independent_variable(sys, sym) && symbolic_type(sym) != NotSymbolic()
end

SymbolicIndexingInterface.default_values(sys::AbstractSystem) = get_defaults(sys)

SymbolicIndexingInterface.is_time_dependent(::AbstractTimeDependentSystem) = true
SymbolicIndexingInterface.is_time_dependent(::AbstractTimeIndependentSystem) = false

Expand Down
2 changes: 1 addition & 1 deletion src/systems/systemstructure.jl
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ function _structural_simplify!(state::TearingState, io; simplify = false,
if has_io
ModelingToolkit.markio!(state, orig_inputs, io...)
end
if io !== nothing
if io !== nothing || any(isinput, state.fullvars)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might (almost certain) have unintended consequences. input = true does not have the same meaning as being part of io.

Copy link
Contributor

@baggepinnen baggepinnen Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for example, all RealInput in MTKstdlib are marked as input = true, but they should not be considered io unless explicitly requested by the user.

Copy link
Member Author

@AayushSabharwal AayushSabharwal Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of this. The fix comes from a test failure: input_output_handling.jl expects the following system to be simplified

@variables x(t) u(t) [input = true]

@named sys = ODESystem([D(x) ~ -x + u], t) # both u and x are unbound

However, without this change the test fails since u isn't recognized as an input. Should the test be updated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I seem to have been a bit confused about how it works internally here, which test was it that failed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssys = structural_simplify(sys)
errors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see

# simplification turns input variables into parameters
ssys = structural_simplify(sys)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few days ago, this code was actually being run unconditionally on io == nothing, so I don't think it will break anything. Also for MTKstdlib, it's probably more correct not run the inputs_to_parameters unless io is provided, in case we want to make more use of the input= true metadata in the future. When the test was written, I think input = true was the only way to indicate an input, but since we added the io argument we changed approach to let the user-provided io indicate the externally visible inputs and outputs, while the input = true metadata lost most of its meaning. the reason is that whether something is an externally visible input is a property of how a component is used, rather than an inherent property of the component. Hence, specifying this information inside component does not make sense, and we instead pass io when we have assembled the whole system

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state, input_idxs = ModelingToolkit.inputs_to_parameters!(state, io)
else
input_idxs = 0:-1 # Empty range
Expand Down
1 change: 1 addition & 0 deletions test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ end
@safetestset "Parsing Test" include("variable_parsing.jl")
@safetestset "Simplify Test" include("simplify.jl")
@safetestset "Direct Usage Test" include("direct.jl")
@safetestset "SymbolicIndeingInterface test" include("symbolic_indexing_interface.jl")
@safetestset "System Linearity Test" include("linearity.jl")
@safetestset "Input Output Test" include("input_output_handling.jl")
@safetestset "Clock Test" include("clock.jl")
Expand Down
5 changes: 4 additions & 1 deletion test/symbolic_indexing_interface.jl
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
using ModelingToolkit, SymbolicIndexingInterface, SciMLBase

@parameters t a b
@variables x(t) y(t)
@variables x(t)=1.0 y(t)=2.0
D = Differential(t)
eqs = [D(x) ~ a * y + t, D(y) ~ b * t]
@named odesys = ODESystem(eqs, t, [x, y], [a, b])
Expand All @@ -21,6 +21,9 @@ eqs = [D(x) ~ a * y + t, D(y) ~ b * t]
@test isequal(independent_variable_symbols(odesys), [t])
@test is_time_dependent(odesys)
@test constant_structure(odesys)
@test !isempty(default_values(odesys))
@test default_values(odesys)[x] == 1.0
@test default_values(odesys)[y] == 2.0

@variables x y z
@parameters σ ρ β
Expand Down