-
-
Notifications
You must be signed in to change notification settings - Fork 241
Support known_disturbance_inputs in symbol-based generate_control_function #4230
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
base: master
Are you sure you want to change the base?
Support known_disturbance_inputs in symbol-based generate_control_function #4230
Conversation
…ction The symbol/analysis-point override of generate_control_function did not forward the known_disturbance_inputs keyword to the base function. This adds the keyword, opens the corresponding loops, and passes the resulting variables through. The disabled doc example in disturbance_modeling.md is re-enabled using the symbol API, and a test is added for the new keyword path. Fixes SciML#4215 Co-Authored-By: Chris Rackauckas <[email protected]> Co-Authored-By: Claude Opus 4.5 <[email protected]>
| op = ModelingToolkit.inputs(io_sys) .=> 0 | ||
| x0 = ModelingToolkit.get_u0(io_sys, op) | ||
| p = MTKParameters(io_sys, op) | ||
| u = zeros(1) # Control input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't remove comments like this
| w = zeros(length(disturbance_inputs)) # Disturbance input (known disturbances are provided as arguments) | ||
| @test f_oop(x0, u, p, t, w) == zeros(5) | ||
| u = zeros(1) | ||
| w = zeros(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't hardcode numbers when the existing code had a better solution
|
|
||
| g = ModelingToolkit.build_explicit_observed_function( | ||
| io_sys, outputs; inputs) | ||
| io_sys, outputs; inputs = ModelingToolkit.inputs(io_sys)[1:1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the existing code was better
Address review feedback: keep descriptive comments from the original example, use `length(disturbance_inputs)` instead of hardcoded `2`, and restore the multi-line formatting style. Co-Authored-By: Chris Rackauckas <[email protected]> Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Addressed all three review comments in 2cb0c23:
|
|
@baggepinnen more comments? |
| P = ssys.system_model | ||
| ```@example DISTURBANCE_MODELING | ||
| inputs = [:u] | ||
| disturbance_inputs = [:d1, :d2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the old syntax of providing analysis points, the tutorial should keep using the new syntax.
The problem is that you should not use the legacy symbol-based API. |
Pass symbols inline to generate_control_function instead of creating intermediate variables with the old analysis-point syntax. Co-Authored-By: Chris Rackauckas <[email protected]> Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Fixed in 93dd2fb: symbols are now passed inline directly to |
Summary
known_disturbance_inputskeyword to the symbol/analysis-point override ofgenerate_control_functioninanalysis_points.jl, opening the corresponding loops and forwarding the variables to the base function.disturbance_modeling.mdusing the symbol-based API ([:d1, :d2]instead of compiled system variables).test_disturbance_model.jlexercising the new keyword path.Fixes #4215
Test plan
test/downstream/test_disturbance_model.jlpassesknown_disturbance_inputswith symbol API (input count, zero/nonzero disturbance behavior)🤖 Generated with Claude Code