refactor: implement new additions to SII interface #2544
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
This might (almost certain) have unintended consequences. input = true does not have the same meaning as being part of io.
There was a problem hiding this comment.
for example, all RealInput in MTKstdlib are marked as input = true, but they should not be considered io unless explicitly requested by the user.
There was a problem hiding this comment.
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 unboundHowever, without this change the test fails since u isn't recognized as an input. Should the test be updated?
There was a problem hiding this comment.
hmm, I seem to have been a bit confused about how it works internally here, which test was it that failed?
There was a problem hiding this comment.
There was a problem hiding this comment.
ah, I see
# simplification turns input variables into parameters
ssys = structural_simplify(sys)There was a problem hiding this comment.
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
Also some bug fixes and test improvements
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.