Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hersle
Copy link
Contributor

@hersle hersle commented Jul 16, 2024

I suggest throwing this nicer error to close #2727

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

@hersle
Copy link
Contributor Author

hersle commented Jul 17, 2024

Not entirely sure if/how to proceed with this.

  • Catalyst uses an equation like parameter ~ independent variable for a continuous event trigger and calls check_equations() manually,
  • ModelingToolkitStandardLibrary does something I don't understand,
  • SDESystem seems to check_equations() on some things that are just expressions (I assume they really correspond to left/right sides of real equations).

The other failures I think I could fix straight-forwardly.

@isaacsas
Copy link
Member

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.

Catalyst.ReactionSystem is a new system type (like ODESystem) that therefore reuses a lot of the internal checks MTK has for such systems (it can, for example also encapsulate ODEs and algebraic equations).

@isaacsas
Copy link
Member

(Also, I think the same error would arise from a similar continuous event condition, parameter ~ t, being passed to an ODESystem, so this isn't really a Catalyst-specific issue -- MTK just doesn't have a test on such a case apparently.)

@ChrisRackauckas
Copy link
Member

@AayushSabharwal is this incompatible with #2747? I guess we could just not apply this to NonlinearSystem?

@hersle
Copy link
Contributor Author

hersle commented Jul 20, 2024

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.

@ChrisRackauckas
Copy link
Member

One of the changes down the line which may happen is that unknowns and parameters may be more fungible by structural simplification. If D(x) ~ 0 then you could move an unknown value to a parameter. One of the things that prevented this in the past is that this can make it so events have issues, since you then need to change the value. But now we also have discrete values, which are functionally stored as parameters but made to be updated and changed during events, and so they are discrete-time state values. In theory then the determination of what is actually unknown could get smarter. I'm not sure if this necessitates a breaking change though, or if this is what would be the core of a v10.

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 fixed=false parameter, where they default to fixed=true, and if it's false then they can be determined by initialization. For more on that, see:

https://mbe.modelica.university/behavior/equations/variables/

The fixed attribute changes the way the start attribute is used when the start attribute is used as an initial condition. Normally, the start attribute is considered a “fallback” initial condition and only used if there are insufficient initial conditions explicitly specified in the initial equation sections. However, if the fixed attribute is set to true, then the start attribute is treated as if it was used as an explicit initial equation (i.e., it is no longer used as a fallback, but instead treated as a strict initial condition).

Another, more obscure, use of the fixed attribute is for “computed parameters”. In rare cases where a parameter cannot be initialized explicitly, it is possible to provide a general equation for the parameter in an initial equation section. But in cases where the parameter is initialized in this way, the fixed attribute for the parameter variable must be set to false.

Default: false (except for parameter variables, where it is true by default)

@AayushSabharwal
Copy link
Member

@AayushSabharwal is this incompatible with #2747? I guess we could just not apply this to NonlinearSystem?

#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

@ChrisRackauckas
Copy link
Member

Rebase this? I think we should go for it.

@ChrisRackauckas ChrisRackauckas force-pushed the forbid_eqs_without_vars branch from f2ab2d0 to 0453e72 Compare July 27, 2024 13:24
@hersle
Copy link
Contributor Author

hersle commented Aug 6, 2024

@ChrisRackauckas

One of the changes down the line which may happen is that unknowns and parameters may be more fungible by structural simplification.

I think this would be fantastic and a step in the right direction.

If D(x) ~ 0 then you could move an unknown value to a parameter.

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 D(x) ~ x, you can also replace it by x ~ A * exp(t), where A must again be a parameter to account for the initial condition, and so on. Maybe this is obvious; I just like to connect the dots 🙂

@AayushSabharwal
Copy link
Member

For now this is fine, but one of my goals is to allow specifying parameter_dependencies as parameter-only equations (enforcing that the LHS is a single parameter) so it'll be removed eventually

@ChrisRackauckas
Copy link
Member

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 D(x) ~ x, you can also replace it by x ~ A * exp(t), where A must again be a parameter to account for the initial condition, and so on. Maybe this is obvious; I just like to connect the dots 🙂

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Structural_simplify not working on tiered model
4 participants