-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Fix parameter dependencies across model hierarchy #2978
Conversation
4a62468
to
9ca10d8
Compare
This is failing CI though EDIT: Wait, |
142aba4
to
4d78d91
Compare
4d78d91
to
861511e
Compare
@ChrisRackauckas I rebased the PR |
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.
Format?
I saw that you're doing it in 2a298d0, so I didn't do it to avoid conflicts 😅 |
Conflicts are a problem for whoever merges last :P Also, in this specific case that commit would just end up being dropped |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Parameter dependencies can get broken if
parameter_dependencies(sys::AbstractSystem)
returnsVector{Any}
, asprocess_parameter_dependencies(pdeps, ps)
checks the element typeModelingToolkit.jl/src/systems/abstractsystem.jl
Lines 2951 to 2953 in 874985c
This PR adds a type assert in equation namespaceing and makes
parameter_dependencies
infer better.One case that shows this issue is having parameter dependencies that reference a subsystem (and this didn't have a test anyway, so I added that).
Note that while #2966 also made changes in this area, the test case that I am adding was not fixed by that. I had an initial version of this PR which would split off the empty case, but after the recursion, but splitting that off should no longer be needed as type inference on mapping
namespace_equation
is better and we should always haveVector{Equation}
.