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

Transform systems robustly by remaking systems #2875

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hersle
Copy link
Contributor

@hersle hersle commented Jul 18, 2024

This is an attempt to strengthen the robustness of system transformations (i.e. processes that turn one system into another).

Internally, as pointed out in #1710 and #1752, there is inconsistency between

  1. transformations that re-calls the system constructor with the modified fields (dropping unspecified fields, but redoing checks),
  2. transformations that uses @set! sys.field = value from Setfield.jl (keeping unspecified fields, but skipping checks).

Field dropping has spawned bugs like #2845 and #2502, which usually gets "duct tape patched" by adding that single field for one particular system type. Transforming systems in a uniform way could prevent such bugs in the future, and maybe simplify some internal machinery. Maybe it would also be more efficient to "batch remake" several modified fields with a single new constructor call, rather than @set! sys.field1 = value1; @set! sys.field2 = value2; ... which I think calls the constructor several times.

Externally, I "envision" that a simple and robust interface could facilitate users making their own custom transformations. Maybe another package using ModelingToolkit would like to build models in a way that requires some simple application-dependent transformation of the system. Every user shouldn't have to reinvent a machinery for doing this themselves.

This is a draft for now. I would really appreciate thoughts and suggestions, or to know if you think this is not a good idea.

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 18, 2024

For now I've tried the remake(sys) functionality out in a few select places. In particular, I "showcase" how it can be used to implement an extend() function that handles all fields of arbitrary system types (hopefully).

I would try to take it further, but first I think it is more important to decide whether this is a good idea at all, and if so to settle the base mechanism 😅

@hersle hersle marked this pull request as draft July 19, 2024 11:12
@ChrisRackauckas
Copy link
Member

I think this is a great idea. Indeed mutation is not a good idea on systems, and a safe remake would help make things more robust for people to do things.

@isaacsas
Copy link
Member

FWIW in Catalyst I came to the conclusion this would be very helpful to have and perhaps reduce errors when transforming systems a little while ago: SciML/Catalyst.jl#935

We would definitely hook into this for ReactionSystems.

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.

3 participants