NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

Documentation updates and testing#1038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

murrayrm merged 15 commits intopython-control:mainfrommurrayrm:doc-comment_fixes-11May2024

Aug 13, 2024

Merged

Documentation updates and testing#1038

murrayrm merged 15 commits intopython-control:mainfrommurrayrm:doc-comment_fixes-11May2024

Aug 13, 2024

Conversation

Copy link

Member

murrayrm commentedAug 9, 2024•
edited
Loading

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

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

murrayrm mentioned this pull request

Aug 9, 2024

Version 0.10.1 release plans#1020

Closed

11 tasks

Copy link

coveralls commentedAug 9, 2024•
edited
Loading

coverage: 94.693% (-0.001%) from 94.694%
when pullingc3ea6dc on murrayrm:doc-comment_fixes-11May2024
into3d79ce3 on python-control:main.

slivingston self-assigned this

Aug 11, 2024

murrayrm added10 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 thedoc-comment_fixes-11May2024 branch fromc9a07c1 to4ef9dd6Compare

August 11, 2024 21:22

slivingston reviewed

Aug 12, 2024

View reviewed changes

Copy link

Member

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

control/ctrlplot.py Outdated

		@@ -220,6 +221,10 @@ def suptitle(
		def get_plot_axes(line_array):
		"""Get a list of axes from an array of lines.

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

Copy link

Member

slivingstonAug 12, 2024

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`toobtainaxesforacontrolplot`cplt`.
	Use`cplt.axes`toobtainaxesforaninstanceof :class:`ControlPlot`.

Even thoughget_plot_axes() is defined in the same module (ctrlplot) asControlPlot, it is available at the top of the package ascontrol.get_plot_axes(), so a user may not realize thatcplt abbreviates the new classControlPlot. E.g., observe howget_plot_axes() is shown athttps://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 classControlPlot.

control/ctrlplot.py Outdated

		@@ -268,6 +274,9 @@ def pole_zero_subplots(
		Scaling to apply to the subplots.
		fig : :class:`matplotlib.figure.Figure`
		Figure to use for creating subplots.
		rcParams : dict
		Override the default parameters used for generating plots.
		Default is set up config.default['freqplot.rcParams'].

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

Suggested change

	Defaultissetupconfig.default['freqplot.rcParams'].
	Defaultissetupconfig.default['ctrlplot.rcParams'].

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

Copy link

Member

slivingstonAug 12, 2024

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 andphase_label should be deleted below.

control/freqplot.py Outdated

		show_legend : bool, optional
		Force legend to be shown if ``True`` or hidden if ``False``. If
		``None``, then show legend when there is more than one line on an
		axis or ``legend_loc`` or ``legend_map`` has been specified.
		title : str, optional
		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

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

Suggested change

	'axes' (default),thehorizontalpositionofthetitlewill
	'axes' (default),thehorizontalpositionofthetitlewillbe

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

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

Suggested change

	centeredrelativetotheaxes.Ifsetto`figure`,itwillbe
	centeredrelativetotheaxes.Ifsetto'figure',itwillbe

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

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

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

Suggested change

	centeredrelativetotheaxes.Ifsetto`figure`,itwillbe
	centeredrelativetotheaxes.Ifsetto'figure',itwillbe

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

slivingston reviewed

Aug 12, 2024

View reviewed changes

Copy link

Member

slivingston left a 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 thenumpydoc repository or made into aruff rule.

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

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

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

Suggested change

	Descriptionofthesystemstates.Sameformatas`inputs`.
	Descriptionofthesystemstates.Sameformatas`outputs`.

Forinputs, we have "Same format asoutputs." Best to just point from here tooutputs (instead ofinputs, which in turn points tooutputs).

control/phaseplot.py Outdated

		@@ -985,6 +1013,9 @@ def phase_plot(odefun, X=None, Y=None, scale=1, X0=None, T=None,

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

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

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

Suggested change

	Thisfunctionisdeprecated;use`phase_plane_plot`instead.
	Thisfunctionisdeprecated;use:func:`phase_plane_plot`instead.

Else, Sphinx/numpydoc will not create a link.

control/phaseplot.py

		@@ -1243,14 +1274,18 @@ def phase_plot(odefun, X=None, Y=None, scale=1, X0=None, T=None,
		def box_grid(xlimp, ylimp):
		"""box_grid generate list of points on edge of box

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

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

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

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(

Copy link

Member

slivingstonAug 12, 2024

Choose a reason for hiding this comment

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

Suggested change

	iff"*{argname}"notindocstringandverbose:
	print(f"{name} has positional arguments; "
	"check manually")
	warnings.warn(
	iff"*{argname}"notindocstring:
	ifverbose:
	print(f"{name} has positional arguments; "
	"check manually")
	warnings.warn(

This warning message should be sent independently ofverbose.

murrayrm added5 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@slivingstonreview comments

fb2086c

filter known FutureWarning's from unit tests

a201504

add back missing image examples/vehicle-steering.png

c3ea6dc

murrayrm merged commitecf6a38 intopython-control:main

Aug 13, 2024

12 checks passed

murrayrm deleted the doc-comment_fixes-11May2024 branch

August 13, 2024 13:32

murrayrm added this to the0.10.1 milestone

Aug 17, 2024

Labels

None yet

3 participants

Movatterモバイル変換

Documentation updates and testing#1038

Documentation updates and testing#1038

Uh oh!

Conversation

murrayrm commentedAug 9, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

coveralls commentedAug 9, 2024• editedLoading 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!

Uh oh!

Uh oh!

murrayrm commentedAug 9, 2024•
edited
Loading

coveralls commentedAug 9, 2024•
edited
Loading