- Notifications
You must be signed in to change notification settings - Fork441
Docstring fix in create_statefbk_iosystem#923
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Docstring fix in create_statefbk_iosystem#923
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Add math keywords to equations, Update some math equations
…h :math:formula, fix typos
Add 'literal block ::' for code line
coveralls commentedJul 13, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 numpydoc style guide says:
Equations : as discussed in theNotes section above, LaTeX formatting should be kept to a minimum. Often it’s possible to show equations as Python code or pseudo-code instead, which is much more readable in a terminal.
Based on this, I have made several suggestions below regarding reverting to plain text in the docstrings, for better display on terminals.
@sawyerbfuller@bnavigator Any thoughts on this general style issue? If we think that Sphinx/readthedocs is the main way people will digest documentation, I am OK with using the :math: directive more liberally.
control/statefbk.py Outdated
| u =ud - K_p (x -xd) - K_iintegral(C x - C x_d) | ||
| .. math ::u =u_d - K_p (x -x_d) - K_i\int(C x - C x_d) |
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 probably OK, but see the general comment regarding keeping equations to a minimum.
control/statefbk.py Outdated
| u =ud - K_p(mu) (x -xd) - K_i(mu)integral(C x - C x_d) | ||
| .. math ::u =u_d - K_p(\mu) (x -x_d) - K_i(\mu)\int(C x - C x_d) | ||
| wheremu represents the scheduling variable. | ||
| where:math:`\mu` represents the scheduling variable. |
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.
As above, it might make more sense here to just use text rather than LaTeX.
| @@ -614,7 +614,7 @@ def create_statefbk_iosystem( | |||
| is given, the output of this system should represent the full state. | |||
| gain : ndarray or tuple | |||
| If an array is given, it represents the state feedback gain (K). | |||
| If an array is given, it represents the state feedback gain (`K`). | |||
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.
I would leave this as K to make it easier to read on a terminal.
control/statefbk.py Outdated
| @@ -623,18 +623,18 @@ def create_statefbk_iosystem( | |||
| If a tuple is given, then it specifies a gain schedule. The tuple | |||
| should be of the form `(gains, points)` where gains is a list of | |||
| gains :math:`K_j` and points is a list of values :math:`\\mu_j` at | |||
| gains :math:`K_j` and points is a list of values :math:`\mu_j` at | |||
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.
I would change this tomu_j (get rid of the :math: altogether).
control/statefbk.py Outdated
| which the gains are computed. The `gainsched_indices` parameter | ||
| should be used to specify the scheduling variables. | ||
| xd_labels, ud_labels : str or list of str, optional | ||
| Set the name of the signals to use for the desired state and | ||
| inputs. If a single string is specified, it should be a | ||
| format string using the variable `i` as an index. Otherwise, | ||
| a list of strings matching the size ofxd andud, | ||
| a list of strings matching the size of:math:`x_d` and:math:`u_d`, |
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.
Suggest leaving as is.
control/statefbk.py Outdated
| the controller is the desired statexd, the desired inputud, and | ||
| the system statex (or state estimatexhat, if an estimator is | ||
| the controller is the desired state:math:`x_d`, the desired input:math:`u_d`, and | ||
| the system state:math:`x` (or state estimate:math:`\hat{x}`, if an estimator is | ||
| given). If value is an integer `q`, the first `q` values of the | ||
| [xd, ud, x] vector are used. Otherwise, the value should be a | ||
| :math:`[x_d, u_d, x]` vector are used. Otherwise, the value should be a | ||
| slice or a list of indices. The list of indices can be specified | ||
| as either integer offsets or as signal names.The default is to | ||
| use the desired statexd. | ||
| as either integer offsets or as signal names. The default is to | ||
| use the desired state:math:`x_d`. |
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.
I would leave all of this unchanged.
control/statefbk.py Outdated
| takes as inputs the desired state`xd`, the desired input | ||
| `ud`, and either the system state `x` or the estimated state | ||
| `xhat`. It outputs the controller action `u` according to the | ||
| takes as inputs the desired state:math:`x_d`, the desired input | ||
| :math:`u_d`, and either the system state:math:`x` or the estimated state | ||
| :math:`\hat{x}`. It outputs the controller action:math:`u` according to the | ||
| formula :math:`u = u_d - K(x - x_d)`. If the keyword |
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.
I would leave this unchanged and convert the formula on line 683 to remove the :math: directive (sou = ud - K(x - xd)).
control/statefbk.py Outdated
| systems takes as inputs the desired trajectory`(xd, ud)` and | ||
| outputs the system state `x` and the applied input `u` | ||
| system takes as inputs the desired trajectory:math:`(x_d, u_d)` and | ||
| outputs the system state:math:`x` and the applied input:math:`u` |
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.
I would remove the :math: directives.
I agree with all your comments there. More complex equations should be set in math and most users will
OTOH we should not go overboard for single symbols. |
| >>> Q = np.eye(2) | ||
| >>> R = np.eye(1) | ||
| >>> | ||
| >>> K, _, _ = ct.lqr(sys,Q,R) |
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.
| >>>K,_,_=ct.lqr(sys,Q,R) | |
| >>>K,_,_=ct.lqr(sys,Q,R) |
| >>> import control as ct | ||
| >>> import numpy as np | ||
| >>> |
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.
np andct are globally implicit
Lines 280 to 282 in42c6fb1
| doctest_global_setup=""" | |
| import numpy as np | |
| import control as ct |
KybernetikJo commentedAug 22, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I was completely unaware of that part of numpydoc, and I'm ok with us not merging it. What should we do with this PR?
I am fine with all three variants. |
Although there is little left after incorporating our comments, I think even minor typo fixes and formatting are worth merging. Keep on working and thanks for your contributions! |
This PR only changes the docstring of create_statefbk_iosystem, makes help more readable.