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

MTKv10 discussion #3204

Open
AayushSabharwal opened this issue Nov 14, 2024 · 11 comments
Open

MTKv10 discussion #3204

AayushSabharwal opened this issue Nov 14, 2024 · 11 comments

Comments

@AayushSabharwal
Copy link
Member

This issue is meant to track and discuss points of action and potential breaking changes for a future MTKv10 release

  • Make @named always wrap symbolic arguments in ParentScope, so they can be passed multiple levels down the hierarchy. This will need more consideration after identifying which tests break upon making this change.
  • Discuss complete semantics
  • Enforce the u0map argument only contains variables, and pmap only parameters. Discretes will need an exception here.
@AayushSabharwal AayushSabharwal added question Further information is requested breaking new-version and removed question Further information is requested labels Nov 14, 2024
@AayushSabharwal
Copy link
Member Author

  • Remove the find/replace stuff in expand_connections. This is only used for MTKStdlib's implementation of AnalysisPoint but that is moved to MTK as of feat: add analysis points #3285.

@baggepinnen
Copy link
Contributor

baggepinnen commented Jan 11, 2025

I'd like to discuss making the syntax

Component(inner_par = outer_par)

Introduce a parameter dependence instead of a "dependent default". I find the current behavior is almost never what I want. In modelica, this syntax introduces the equivalent of a parameter dependence.

More generally, support for redeclarstion and modification like
https://specification.modelica.org/master/inheritance-modification-and-redeclaration.html (and also JSML)
would be very welcome.

@vyudu
Copy link
Member

vyudu commented Jan 16, 2025

Possible changes to field names in symbolic metadata:

  • vartype becoming variable_source
  • new field variable_type (corresponding to the MTK variable type)

@AayushSabharwal
Copy link
Member Author

^ This specifically refers to the NamedTuple returned from get_variable_metadata

@AayushSabharwal
Copy link
Member Author

Other discussions worth having:

  • defaults are now just soft constraints that are only respected sometimes. Especially for parameters since they have to opt-in to initialization.
  • Not relying on variable metadata as much. This refers to multiple things
    • @mtkmodel macro using it for keeping track of defaults
    • More importantly, structural_simplify using it to figure out what is a parameter and what isn't. ODESystem can use the metadata to do this, but structural_simplify should respect whatever bucket we put it in.

@hersle
Copy link
Contributor

hersle commented Jan 16, 2025

Discuss complete semantics

Should complete() be changed to use flatten = false by default? I find it undesireable that the current default flatten = true destroys the hierarchical model structure without me explicitly requesting it. There are some bugs with flatten = false, though (#3322).

@hersle
Copy link
Contributor

hersle commented Jan 16, 2025

More importantly, structural_simplify using it to figure out what is a parameter and what isn't. ODESystem can use the metadata to do this, but structural_simplify should respect whatever bucket we put it in.

I always thought it could be simpler to have just the @variables macro, and that it should be sufficient to write

@variables t # instead of @independent_variables
vars = @variables x(t)
pars = @variables P # instead of @parameters
eqs = [...]
@named sys = ODESystem(eqs, t, vars, pars)

because it should be clear from the arguments to ODESystem what the independent variable, unknowns and parameters are. Maybe this breaks stuff with the "convenience dispatch" ODESystem(eqs, t) and the @mtkmodel macro (which I have less experience with), or causes other problems, though. This is not a huge deal in practice, though, and I apologize if this is very naive suggestion 😅

@AayushSabharwal
Copy link
Member Author

Maybe this breaks stuff with the "convenience dispatch" ODESystem(eqs, t)

It doesn't break stuff, but it makes catching user errors more difficult. Maybe they accidentally create @variables x intending it to be an unknown?

the @mtkmodel macro

@mtkmodel does its own parsing (something I strongly dislike) so that wouldn't be an issue

or causes other problems, though.

It causes problems for NonlinearSystem, for example, where @variables x could just as well be a parameter as an unknown without the metadata.

@AayushSabharwal
Copy link
Member Author

#3335 (comment)

@ChrisRackauckas
Copy link
Member

defaults are now just soft constraints that are only respected sometimes.

But they are always enforced at the start of a solve?

More importantly, structural_simplify using it to figure out what is a parameter and what isn't. ODESystem can use the metadata to do this, but structural_simplify should respect whatever bucket we put it in.

yes, because otherwise it couldn't handle D(x) ~ 0 turning into a parameter.

@AayushSabharwal
Copy link
Member Author

#3354

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

No branches or pull requests

5 participants