GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — add reference gain design pattern to create_statefbk_iosystem by murrayrm · Pull Request #1071 · python-control/python-control Conversation
|
|
coverage: 94.737% (+0.02%) from 94.721% |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I will review this today. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| (proportional and integral) are evaluated using the scheduling | ||
| variables specified by `gainsched_indices`. | ||
| Input/output system representing the controller. For the 'trajgen' | ||
| design patter (default), this system takes as inputs the desired |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| design patter (default), this system takes as inputs the desired | |
| design pattern (default), this system takes as inputs the desired |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| state `x_d`, the desired input `u_d`, and either the system state | ||
| `x` or the estimated state `xhat`. It outputs the controller | ||
| action `u` according to the formula `u = u_d - K(x - x_d)`. For | ||
| the 'refgain' design patter, the system takes as inputs the |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| the 'refgain' design patter, the system takes as inputs the | |
| the 'refgain' design pattern, the system takes as inputs the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| ctrl, clsys = ct.create_statefbk_iosystem(sys, K, kf, feedfwd_pattern='refgain') | ||
|
|
||
| This reference gain design pattern is described in more detail in Section | ||
| 7.2 of FBS2e (Stabilization by State Feedback) and the trajectory |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I might have missed something, but some readers will not know the reference "FBS2e". One idea is to make FBS2e_ a link to the book website. This can be done with the following changes:
diff --git a/doc/conf.py b/doc/conf.py index 75981d6..a523713 100644 --- a/doc/conf.py +++ b/doc/conf.py @@ -285,3 +285,7 @@ import control.flatsys as fs import control.phaseplot as pp ct.reset_defaults() """ + +rst_prolog = """ +.. _FBS2e: https://fbswiki.org/wiki/index.php/Feedback_Systems:_An_Introduction_for_Scientists_and_Engineers +""" diff --git a/doc/iosys.rst b/doc/iosys.rst index e67f4dc..413f4d4 100644 --- a/doc/iosys.rst +++ b/doc/iosys.rst @@ -57,7 +57,7 @@ Example To illustrate the use of the input/output systems module, we create a model for a predator/prey system, following the notation and parameter -values in FBS2e. +values in FBS2e_. We begin by defining the dynamics of the system
Feel free to apply this change to your PR, or I can open another PR that adds this and possibly other docs enhancements.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| else: | ||
| ctrl, clsys = ct.create_statefbk_iosystem( | ||
| sys, K, Kf, feedfwd_pattern='refgain') |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
create_statefbk_iosystem() is a big function now with several different ways of being called and a docstring of 180 lines. One design pattern to handle this is changing the function to be internal and then creating user-facing functions that have names implying values of arguments like feedfwd_pattern; e.g., create_statefbk_trajgen(), which is implemented by calling _create_statefbk_iosystem() with feedfwd_pattern='trajgen'. However, this alternative may not help users in practice.
In the scope of this PR, it suffices to ensure that mistakes when calling create_statefbk_iosystem() are caught. To that end, can you add a case for when feedfwd_pattern has an unknown value? E.g.,
| sys, K, Kf, feedfwd_pattern='refgain') | |
| sys, K, Kf, feedfwd_pattern='refgain') | |
| with pytest.raises(NotImplementedError, match="unknown pattern"): | |
| ct.create_statefbk_iosystem(sys, K, Kf, feedfwd_pattern='refgai') |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Related to the above, there may be existing code that calls create_statefbk_iosystem() with 3 positional arguments, thus giving integral_action without using the keyword. With this PR, that would effectively be a call to create_statefbk_iosystem() with a value to feedfwd_gain but feedfwd_pattern still at its default. I modified the test test_lqr_integral_continuous to demonstrate this case, and the resulting error is
if isinstance(gain, np.ndarray):
K = gain
if K.shape != (sys_ninputs, estimator.noutputs + nintegrators):
> raise ControlArgument(
f'control gain must be an array of size {sys_ninputs}'
f' x {sys_nstates}' +
(f'+{nintegrators}' if nintegrators > 0 else ''))
E control.exception.ControlArgument: control gain must be an array of size 2 x 4
control/statefbk.py:831: ControlArgument
It is good that an exception is raised, but users may be confused about the cause.
I think that having feedfwd_gain as the third parameter is intuitive and worth keeping, so I propose that a ControlArgument exception is raised if feedfwd_gain is not None but feedfwd_pattern != 'refgain'.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I've left the single function for now, though I agree this function is getting big and complicated. I'm not sure that splitting it in two simplifies things much, and the current naming matches the create_estimator_iosystem and create_mpc_iosystem naming pattern (and functionality).
I created the exception suggested above + unit test.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| (1, 1), (1, None), | ||
| (2, np.diag([1, 1])), (2, None), |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| (1, 1), (1, None), | |
| (2, np.diag([1, 1])), (2, None), | |
| (1, 1), | |
| (1, None), | |
| (2, np.diag([1, 1])), | |
| (2, None), |
Easier to read if we have one case (i.e., one assignment of parameters) per line
Sorry, something went wrong.
There was an error while loading. Please reload this page.
604a3df to
96b7b73
Compare
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.