-
-
Notifications
You must be signed in to change notification settings - Fork 232
feat: more robust inputs/outputs handling #3795
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
1ff0c42
to
3a45833
Compare
This PR would benefit from running DyadControlSystems tests when it's ready |
3a45833
to
3542211
Compare
eqs = [D(x) ~ x + y + z | ||
y ~ z] | ||
@named sys = System(eqs, t) | ||
@test issetequal(ModelingToolkit.inputs(sys), [y]) |
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.
Is this the PR that should address the ordering an number of inputs after mtkcompile(..., inputs = [u1,u2])
? If so, it would be nice to include a test case where this failed
https://github.com/JuliaComputing/DyadControlSystems.jl/actions/runs/17095167938/job/48477801369?pr=644#step:7:1727
I notice also that the build that was triggered still fails in almost the same place, but it now has a different error message than it had before
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 ordering was already fixed in #3804. We have an assertion for the ordering being maintained in complete
, so if by chance something goes wrong it will surface as an error. I don't know how we would test it, since there isn't a specific condition where the ordering goes wrong. It's just that if we're not careful it might get shuffled around, which is what the assertion detects.
The new failure in DyadControlSystems seems to me like a missing splat operation. It's trying to pass a NamedTuple
of 4 matrices where the function expects them as 4 different arguments.
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.
Ah, so it has been a change in MTKv10 that went unnoticed before this fix then, I'll fix it separately.
No description provided.