Docstring fix in create_statefbk_iosystem by KybernetikJo · Pull Request #923 · python-control/python-control

KybernetikJo · 2023-07-13T21:20:52Z

This PR only changes the docstring of create_statefbk_iosystem, makes help more readable.

Changes math formulas to sphinx math
Changes most signal-names to inline math in order to be consistent with formulas
Fixes some typos
Adds a full example

Add math keywords to equations, Update some math equations

…h :math:formula, fix typos

Add 'literal block ::' for code line

coveralls · 2023-07-13T21:31:38Z

coverage: 94.968%. remained the same when pulling e65750a on KybernetikJo:doc-fix-create_statefbk_iosystem into 42c6fb1 on python-control:main.

murrayrm

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.

murrayrm · 2023-08-19T18:14:07Z

control/statefbk.py

    state feedback controller of the form

-        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)


This is probably OK, but see the general comment regarding keeping equations to a minimum.

murrayrm · 2023-08-19T18:14:32Z

control/statefbk.py

-        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.


As above, it might make more sense here to just use text rather than LaTeX.

murrayrm · 2023-08-19T18:15:06Z

control/statefbk.py


    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`).


I would leave this as K to make it easier to read on a terminal.

murrayrm · 2023-08-19T18:15:40Z

control/statefbk.py

        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


I would change this to mu_j (get rid of the :math: altogether).

murrayrm · 2023-08-19T18:15:52Z

control/statefbk.py

        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`,


Suggest leaving as is.

murrayrm · 2023-08-19T18:16:15Z

control/statefbk.py

-        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`.


I would leave all of this unchanged.

murrayrm · 2023-08-19T18:18:08Z

control/statefbk.py

        Input/output system representing the controller.  This system
-        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


I would leave this unchanged and convert the formula on line 683 to remove the :math: directive (so u = ud - K(x - xd)).

murrayrm · 2023-08-19T18:18:22Z

control/statefbk.py

-        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`


I would remove the :math: directives.

bnavigator · 2023-08-21T20:38:18Z

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.

I agree with all your comments there.

More complex equations should be set in math and most users will

use some sort of Sphinx (RTD or inline help in IDEs like Spyder, JupyterLab might get it in the future too: Use sphinx to render docstring in contextual help jupyterlab/jupyterlab#8581), or
will understand to read LaTeX, having control systems scientists as our main audience.

OTOH we should not go overboard for single symbols.

bnavigator · 2023-08-21T20:54:17Z

control/statefbk.py

+    >>> Q = np.eye(2)
+    >>> R = np.eye(1)
+    >>>
+    >>> K, _, _ = ct.lqr(sys,Q,R)


Suggested change

>>> K, _, _ = ct.lqr(sys,Q,R)

>>> K, _, _ = ct.lqr(sys, Q, R)

bnavigator · 2023-08-21T20:56:20Z

control/statefbk.py

+    >>> import control as ct
+    >>> import numpy as np
+    >>>


np and ct are globally implicit

python-control/doc/conf.py

Lines 280 to 282 in 42c6fb1

doctest_global_setup = """

import numpy as np

import control as ct

KybernetikJo · 2023-08-22T17:57:20Z

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.

I was completely unaware of that part of numpydoc, and I'm ok with us not merging it.
(It was a good remark, because I would like to change Slycot/slycot/analysis.py to numpydoc. I can take that into account over there.)

What should we do with this PR?

Close this PR with comment.
Keep it open, not do anything, just keep it as reference. Maybe latex rendering in terminals will change at some point in the future.
Incorporate comments from @murrayrm, @bnavigator and merge. I'm not sure it's worth it, though.

I am fine with all three variants.

bnavigator · 2023-08-22T19:29:07Z

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!

KybernetikJo added 4 commits July 13, 2023 18:56

Add math keywords to equations, Update some math equations

421cfaa

Add math keywords to equations, Update some math equations

Change most/all signals x,xd,xhat,u,ud to :math: to be consistent wit…

30102bf

…h :math:formula, fix typos

Add 'literal block ::' for code line

dfc8b73

Add 'literal block ::' for code line

Add full example to create_statefbk_iosystem

d0aa48a

murrayrm reviewed Aug 19, 2023

View reviewed changes

bnavigator reviewed Aug 21, 2023

View reviewed changes

murrayrm added 2 commits September 16, 2023 09:30

remove unneeded :math: directives

3950f67

fix up equations in stochsys

e65750a

murrayrm merged commit 0a6146b into python-control:main Sep 16, 2023

murrayrm added this to the 0.10.0 milestone Mar 31, 2024

	>>> K, _, _ = ct.lqr(sys,Q,R)
	>>> K, _, _ = ct.lqr(sys, Q, R)

	doctest_global_setup = """
	import numpy as np
	import control as ct

Conversation

KybernetikJo commented Jul 13, 2023

Uh oh!

coveralls commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnavigator commented Aug 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KybernetikJo commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnavigator commented Aug 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

coveralls commented Jul 13, 2023 •

edited

Loading

KybernetikJo commented Aug 22, 2023 •

edited

Loading