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

feat: add substitute_component #3502

Merged

Conversation

AayushSabharwal
Copy link
Member

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

Additional context

Add any other context about the problem here.

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Mar 27, 2025

I agree that that would be a great syntax. However, we use Setfield internally to update systems and this might conflict with that. Additionally, the setproperties approach would only go one level deep. If a user did @set sys.inner.component.child = newchild, this would first replace child in component, then use that to replace component in inner, then use that to replace inner in sys which is a lot of unnecessary work.

oldargs = [ap.input; ap.outputs]
end
newargs = map(get_systems(eq.rhs)) do arg
name = arg isa AbstractSystem ? nameof(arg) : getname(arg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this locally a bit and I have a case where eq.rhs is a (top level) connect equation and the args are ModelingToolkit.SymbolicWithNameof and then getname(arg) gives the Symbol("var_name(t)") variant instead of just :var_name. I saw that using getname(arg.var) works, but I'm not sure if that makes sense since the resulting system crashes on printing during n_expanded_connection_equations 😅.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops, I always forget that SymbolicWithNameof exists. Thanks for pointing it out.

@AayushSabharwal AayushSabharwal force-pushed the as/substitute-component branch from b5fb23c to 2d9d4e1 Compare March 30, 2025 19:12
@cstjean
Copy link
Contributor

cstjean commented Mar 31, 2025

We could really use this functionality, but unless I'm mistaken the requirements limit its applicability.

  1. rhs must contain at least all of the unknowns and parameters present in lhs.

Eg. "What if we replaced this continuously-stirred tank with a non-stirred tank?" might not meet these requirements, right? Or "ComplexReactorModel => NaiveReactorModel".

I understand that there's probably an efficiency reason for these constraints, but in this case I would rather have inefficient-and-general than efficient-but-limited.

My use case is for running experiments. "What if we replaced component A.B.C with C2(...)?" is much nicer to write than copy-paste the A and B @mtkmodel definitions and reconnect everything.

Is there another way of doing this?

@AayushSabharwal
Copy link
Member Author

I understand that there's probably an efficiency reason for these constraints, but in this case I would rather have inefficient-and-general than efficient-but-limited.

It's not an efficiency but correctness issue. It's really difficult to see how a component (and all subcomponents) are used across the entire model and then verify that the replacement has the same pieces. I'd be more than happy to work on it in the future, but this addresses a limited set of problems with scope for expansion in the future.

The way to do this is define the model with an "interface component" - one which isn't intended for simulation but has the bare minimum interface required to connect with the rest of the system. This can then be replaced to form different model variants.

@AayushSabharwal AayushSabharwal force-pushed the as/substitute-component branch from 2d9d4e1 to e747405 Compare April 2, 2025 06:17
@ChrisRackauckas ChrisRackauckas merged commit d54449d into SciML:master Apr 3, 2025
42 of 44 checks passed
@AayushSabharwal AayushSabharwal deleted the as/substitute-component branch April 4, 2025 06:44
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.

4 participants