-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
Check that equations contain at least one variable #2865
base: master
Are you sure you want to change the base?
Conversation
Not entirely sure if/how to proceed with this.
The other failures I think I could fix straight-forwardly. |
It seems reasonable one might want an event condition to be when time is equal to a parameter. We could flip the order, but given that independent variables are now being treated as parameters you would still have an equation about equality of two parameters.
|
(Also, I think the same error would arise from a similar continuous event condition, |
@AayushSabharwal is this incompatible with #2747? I guess we could just not apply this to NonlinearSystem? |
That also struck my mind, so maybe this check is not a great idea to add now. One could also argue that it should be legal to have parameter-only equations even in the "main equations" of an ODE system. To be forgiving, MTK could detect them and automatically move them to the initialization problem. |
One of the changes down the line which may happen is that unknowns and parameters may be more fungible by structural simplification. If But if that's the case, it really change this PR and the other parameter initialization one to be much better handled by "the normal flow". Now one other thing to mention, Modelica has a notion of a https://mbe.modelica.university/behavior/equations/variables/
|
#2747 is too old to merge, and will basically need to be redone. It shouldn't really factor in to the design decisions we make. Allowing parameters to be unknowns in the initialization is something that can be done regardless of this PR |
Rebase this? I think we should go for it. |
f2ab2d0
to
0453e72
Compare
I think this would be fantastic and a step in the right direction.
It is very insightful to understand this as a special case of an even more powerful feature: structural simplification replacing equations by their analytical solutions (e.g. #1586). If you had |
For now this is fine, but one of my goals is to allow specifying |
Yup, we need to do a few other analytical solutions first, but it has already started JuliaSymbolics/Symbolics.jl#1192 Once that's in, integrating analytical solutions to rootfinding into the numeric stack is first, and then should do integrals and ODEs. |
I suggest throwing this nicer error to close #2727
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.