GeezerVison

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

GeezerVison is showing github.com — Documentation updates and testing by murrayrm · Pull Request #1038 · python-control/python-control

Conversation

murrayrm commented Aug 9, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

This PR provides updated docstrings for a large number of functions that had undocumented arguments, along with a unit test that checks to make sure that positional arguments and keyword arguments are appropriately documented. The unit test (docstring_test.py) checks for the following:

All named arguments should be documented in the docstring using standard Python docstring syntax (parameter name followed by a space, then a colon, then another space).
Detectable keyword arguments accessed via **kwargs are also checked to make sure they are documented.
Exceptions are allowed to skip testing for entire functions or specific arguments in selected functions.
Functions with unnamed positional arguments (*args) must be checked manually and an MD5 hash on the source code is used to make sure that changes will require reconfirmation (by updating the stored hash).
Deprecated functions must contain a Sphynx deprecation message and issue a FutureWarning.

Implementing this unit test picked up ~30 undocumented variables that are now documented, as well as triggering a few changes in argument processing and some argument hiding (for internal arguments that the user should never call, which now should start with an underscore).

This PR contains no changes in functionality, just docstring updates and small code changes for consistency (eg, DeprecationWarning [which is ignored/hidden by default] → FutureWarning [which generates a user-visible warning]).

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm mentioned this pull request

Aug 9, 2024

Version 0.10.1 release plans #1020

Closed

11 tasks done

coveralls commented Aug 9, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

coverage: 94.693% (-0.001%) from 94.694%
when pulling c3ea6dc on murrayrm:doc-comment_fixes-11May2024
into 3d79ce3 on python-control:main.

All reactions

Sorry, something went wrong.

Uh oh!

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

slivingston self-assigned this

Aug 11, 2024

murrayrm added 10 commits

August 11, 2024 14:18


          small updates to comments + remove obsolete Pending

e01c580


          docstring updates for nyquist_plot

f078b3e


          add unit test for testing documentation

4838bbf


          change check_kwargs to _check_kwargs and disable docstring check

b767e52


          improve docstrings unit test for deprecation, seq args; skip duplicates

3697afa


          add hashes for positional arguments

a59184c


          update intersphinx URL for SciPy

4d9f57d


          update docstrings to fix missing arguments, incorrect format

7f48535


          make deprecration warnings consistent (as FutureWarning)

8fcf0b7


          update docstring text for new functions

4ef9dd6

murrayrm force-pushed the doc-comment_fixes-11May2024 branch from c9a07c1 to 4ef9dd6 Compare

August 11, 2024 21:22

slivingston reviewed

Aug 12, 2024

View reviewed changes

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

I plan to finish review tomorrow.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/ctrlplot.py Outdated


		.. deprecated:: 0.10.1
		This function will be removed in a future version of python-control.
		Use `cplt.axes` to obtain axes for a control plot `cplt`.

slivingston Aug 12, 2024

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.

Suggested change

	Use `cplt.axes` to obtain axes for a control plot `cplt`.
	Use `cplt.axes` to obtain axes for an instance of :class:`ControlPlot`.

Even though get_plot_axes() is defined in the same module (ctrlplot) as ControlPlot, it is available at the top of the package as control.get_plot_axes(), so a user may not realize that cplt abbreviates the new class ControlPlot. E.g., observe how get_plot_axes() is shown at https://python-control.readthedocs.io/en/0.10.0/generated/control.get_plot_axes.html.

Therefore, I recommend to be explicit here when referring to the class ControlPlot.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/ctrlplot.py Outdated

		Figure to use for creating subplots.
		rcParams : dict
		Override the default parameters used for generating plots.
		Default is set up config.default['freqplot.rcParams'].

slivingston Aug 12, 2024

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.

Suggested change

	Default is set up config.default['freqplot.rcParams'].
	Default is set up config.default['ctrlplot.rcParams'].

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/freqplot.py

		Frequency label (defaults to "rad/sec" or "Hertz")
		freq_label, magnitude_label, phase_label : str, optional
		Labels to use for the frequency, magnitude, and phase axes.
		Defaults are set by `config.defaults['freqplot.<keyword>']`.

slivingston Aug 12, 2024

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 like that descriptions of these parameter are combined, but in this case, magnitude_label and phase_label should be deleted below.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/freqplot.py Outdated

		Set the title of the plot. Defaults to plot type and system name(s).
		title_frame : str, optional
		Set the frame of reference used to center the plot title. If set to
		'axes' (default), the horizontal position of the title will

slivingston Aug 12, 2024

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.

Suggested change

	'axes' (default), the horizontal position of the title will
	'axes' (default), the horizontal position of the title will be

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/freqplot.py Outdated

		title_frame : str, optional
		Set the frame of reference used to center the plot title. If set to
		'axes' (default), the horizontal position of the title will
		centered relative to the axes. If set to `figure`, it will be

slivingston Aug 12, 2024

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.

Suggested change

	centered relative to the axes. If set to `figure`, it will be
	centered relative to the axes. If set to 'figure', it will be

To be consistent with 'axes' and other strings in these parameter descriptions, this should be 'figure' rather than `figure`.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/freqplot.py Outdated

		title_frame : str, optional
		Set the frame of reference used to center the plot title. If set to
		'axes' (default), the horizontal position of the title will
		centered relative to the axes. If set to `figure`, it will be

slivingston Aug 12, 2024

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.

Suggested change

	centered relative to the axes. If set to `figure`, it will be
	centered relative to the axes. If set to 'figure', it will be

To be consistent with 'axes' and other strings in these parameter descriptions, this should be 'figure' rather than figure.

Sorry, something went wrong.

Uh oh!

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

All reactions

slivingston reviewed

Aug 12, 2024

View reviewed changes

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

Excellent work on the docstrings checker. It seems useful enough to be offered to the numpydoc repository or made into a ruff rule.

I added several more small comments. After considering those, this is ready to merge.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/statesp.py Outdated

		inputs : int, list of str, or None
		Description of the system inputs. This can be given as an integer
		states : int, list of str, or None
		Description of the system states. Same format as `inputs`.

slivingston Aug 12, 2024

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.

Suggested change

	Description of the system states. Same format as `inputs`.
	Description of the system states. Same format as `outputs`.

For inputs, we have "Same format as outputs." Best to just point from here to outputs (instead of inputs, which in turn points to outputs).

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/phaseplot.py Outdated

		"""(legacy) Phase plot for 2D dynamical systems.

		.. deprecated:: 0.10.1
		This function is deprecated; use `phase_plane_plot` instead.

slivingston Aug 12, 2024

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.

Suggested change

	This function is deprecated; use `phase_plane_plot` instead.
	This function is deprecated; use :func:`phase_plane_plot` instead.

Else, Sphinx/numpydoc will not create a link.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/phaseplot.py

		"""box_grid generate list of points on edge of box

		.. deprecated:: 0.10.0
		Use :func:`phaseplot.boxgrid` instead.

slivingston Aug 12, 2024

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.

Note that neither boxgrid() nor box_grid() appear in the generated user's manual, so this :func: link is not used anywhere that I observed.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/tests/docstrings_test.py Outdated

Comment on lines +146 to +149

		if f"*{argname}" not in docstring and verbose:
		print(f" {name} has positional arguments; "
		"check manually")
		warnings.warn(

slivingston Aug 12, 2024

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.

Suggested change

	if f"*{argname}" not in docstring and verbose:
	print(f" {name} has positional arguments; "
	"check manually")
	warnings.warn(
	if f"*{argname}" not in docstring:
	if verbose:
	print(f" {name} has positional arguments; "
	"check manually")
	warnings.warn(

This warning message should be sent independently of verbose.

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm added 5 commits

August 12, 2024 21:58


          add test for multiple copies of argument documentation

806e5b3


          update docstring position arg hash to include source + docstring

122c405


          address @slivingston review comments

fb2086c


          filter known FutureWarning's from unit tests

a201504


          add back missing image examples/vehicle-steering.png

c3ea6dc

murrayrm merged commit ecf6a38 into python-control:main

Aug 13, 2024

murrayrm deleted the doc-comment_fixes-11May2024 branch

August 13, 2024 13:32

murrayrm added this to the 0.10.1 milestone

Aug 17, 2024

Reviewers

slivingston

slivingston left review comments

Uh oh!

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

Assignees

slivingston

Labels

None yet

Projects

None yet

Milestone

0.10.1

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

3 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 01402520 — 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

murrayrm commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

murrayrm commented Aug 9, 2024 •

edited

Loading

Uh oh!

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

coveralls commented Aug 9, 2024 •

edited

Loading

Uh oh!

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