Documentation updates and testing #1038

murrayrm · 2024-08-09T04:03:17Z

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

coveralls · 2024-08-09T04:18:21Z

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

slivingston

I plan to finish review tomorrow.

slivingston · 2024-08-12T03:55:07Z

control/ctrlplot.py


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


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.

slivingston · 2024-08-12T03:56:14Z

control/ctrlplot.py

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


Suggested change

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

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

slivingston · 2024-08-12T04:25:57Z

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


I like that descriptions of these parameter are combined, but in this case, magnitude_label and phase_label should be deleted below.

slivingston · 2024-08-12T04:26:57Z

control/freqplot.py

        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


Suggested change

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

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

slivingston · 2024-08-12T04:29:49Z

control/freqplot.py

+    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


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

slivingston · 2024-08-12T04:53:24Z

control/freqplot.py

+    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


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.

slivingston

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.

slivingston · 2024-08-12T06:44:48Z

control/statesp.py

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


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

slivingston · 2024-08-12T18:30:48Z

control/phaseplot.py

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

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


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.

slivingston · 2024-08-12T18:39:55Z

control/phaseplot.py

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

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


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.

slivingston · 2024-08-12T18:52:02Z

control/tests/docstrings_test.py

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


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.

murrayrm mentioned this pull request Aug 9, 2024

Version 0.10.1 release plans #1020

Closed

11 tasks

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

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

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

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

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

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

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

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

Documentation updates and testing #1038

Documentation updates and testing #1038

Uh oh!

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

murrayrm commented Aug 9, 2024 •

edited

Loading

coveralls commented Aug 9, 2024 •

edited

Loading