Docstring fix in create_statefbk_iosystem by KybernetikJo · Pull Request #923

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

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

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

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

		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)

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.

Comment on lines -606 to +608

		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.

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.


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

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.

		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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest leaving as is.

Comment on lines +653 to +659

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

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.

Comment on lines 680 to 683

		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

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 (so u = ud - K(x - xd)).

Comment on lines +693 to +694

		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`

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.

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.

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

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)

Comment on lines +726 to +728

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np and ct are globally implicit

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

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.

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!