Give a warning for underconstrained variables forced to 0 #2328
Conversation
|
I'm not sure this makes sense here? I would think the check should be done on the |
|
How would that call know? This information is discovered during bareiss. |
| function alias_eliminate_graph!(state::TransformationState, ils::SparseMatrixCLIL) | ||
| function force_var_to_zero!(structure::SystemStructure, ils::SparseMatrixCLIL, v::Int) | ||
| @unpack graph, solvable_graph, eq_to_diff = structure | ||
| @set! ils.nparentrows += 1 |
There was a problem hiding this comment.
This call needs to return ils because @set! creates a new object.
There was a problem hiding this comment.
Good catch, I thought it was mutable, but of course it isn't.
| @@ -1,14 +1,15 @@ | |||
| using SymbolicUtils: Rewriters | |||
| using Graphs.Experimental.Traversals | |||
|
|
|||
| function alias_eliminate_graph!(state::TransformationState; kwargs...) | |||
| function alias_eliminate_graph!(state::TransformationState; | |||
There was a problem hiding this comment.
We need to make warning reporting an option here, because sometimes MTK performs transformations that leave harmless dangling variables.
There was a problem hiding this comment.
The default here is not to warn. The warning is in the higher level function in alias_elimination! that has access to the variables. If you can tell which variables are harmless, you can suppress the warning there.
@oxinabox noticed that the following MSL test https://github.com/SciML/ModelingToolkitStandardLibrary.jl/blob/0f0fae138fe202140a4862eccaaac734051dc44f/test/Mechanical/translational.jl has a test that depends on an underconstrained variable (either `free.f` or `acc.f` may be set to an arbitrary value without affecting the dynamics of the system). Our structural singularity removal logic detects these situations and chooses one variable arbitrarily to set to 0. However, it does so silently, so users are unaware that they likely have a modeling bug. This PR adds a warning when this happens. For example, the above mentioned test now gives: ``` ┌ Warning: The model is under-constrained. Variable acc₊flange₊f(t) was arbitrarily chosen to be set to 0. This may indicate a model bug! └ @ ModelingToolkit ~/.julia/dev/ModelingToolkit/src/systems/alias_elimination.jl:54 ``` This is styled as an overridable callback, so higher level modeling frameworks that use MTK as a library can hook into this to give more domain-specific errors if desired.
Keno
force-pushed
the
kf/unconstrainedwarning
branch
from
a193152 to
e34f3de
Compare
|
Bump on this? Came up again. |
|
Looks like we need to update the tests as well https://github.com/SciML/ModelingToolkit.jl/actions/runs/8443829319/job/23128226047?pr=2328#step:6:1037 |
@oxinabox noticed that the following MSL test
https://github.com/SciML/ModelingToolkitStandardLibrary.jl/blob/0f0fae138fe202140a4862eccaaac734051dc44f/test/Mechanical/translational.jl
has a test that depends on an underconstrained variable (either
free.foracc.fmay be set to an arbitrary value without affecting the dynamics of the system). Our structural singularity removal logic detects these situations and chooses one variable arbitrarily to set to 0. However, it does so silently, so users are unaware that they likely have a modeling bug. This PR adds a warning when this happens. For example, the above mentioned test now gives:This is styled as an overridable callback, so higher level modeling frameworks that use MTK as a library can hook into this to give more domain-specific errors if desired.