Docstring fix in create_statefbk_iosystem #923
Conversation
Add math keywords to equations, Update some math equations
…h :math:formula, fix typos
Add 'literal block ::' for code line
There was a problem hiding this comment.
The numpydoc style guide says:
Equations : as discussed in the Notes 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_i integral(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.
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) | ||
|
|
||
| where mu represents the scheduling variable. | ||
| where :math:`\mu` represents the scheduling variable. |
There was a problem hiding this comment.
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.
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.
I would change this to mu_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 of xd and ud, | ||
| a list of strings matching the size of :math:`x_d` and :math:`u_d`, |
control/statefbk.py
Outdated
| the controller is the desired state xd, the desired input ud, and | ||
| the system state x (or state estimate xhat, 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 state xd. | ||
| 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.
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.
I would leave this unchanged and convert the formula on line 683 to remove the :math: directive (so u = 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.
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.
| >>> 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.
np and ct are globally implicit
Lines 280 to 282 in 42c6fb1
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.