GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Docstring fix in create_statefbk_iosystem by KybernetikJo · Pull Request #923 · python-control/python-control

Conversation

KybernetikJo commented Jul 13, 2023

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

coveralls commented Jul 13, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm reviewed

Aug 19, 2023

View reviewed changes

murrayrm left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/statefbk.py Outdated

		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)

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/statefbk.py

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.

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/statefbk.py Outdated

		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

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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 to mu_j (get rid of the :math: altogether).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/statefbk.py Outdated

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

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/statefbk.py Outdated

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

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/statefbk.py Outdated

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

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/statefbk.py Outdated

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`

murrayrm Aug 19, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator commented Aug 21, 2023

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator reviewed

Aug 21, 2023

View reviewed changes

Comment thread

control/statefbk.py

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

bnavigator Aug 21, 2023

Copy link

Copy Markdown

Contributor

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.

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator reviewed

Aug 21, 2023

View reviewed changes

Comment thread

control/statefbk.py

Comment on lines +726 to +728

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

bnavigator Aug 21, 2023

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

KybernetikJo commented Aug 22, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator commented Aug 22, 2023

Copy link

Copy Markdown

Contributor

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

murrayrm

murrayrm left review comments

bnavigator

bnavigator left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.10.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jul 13, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

KybernetikJo commented Aug 22, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.