Updated documentation by murrayrm · Pull Request #1094 · python-control/python-control
Conversation
This (very large) PR contains a restructuring of the Sphinx documentation, including changes to improve uniformity of docstrings along with draft guidelines for developers. This is being posted initially as a draft PR so that people can start to have a look at the changes and provide feedback.
High level principles:
- No documentation has been removed, only shifted around.
- With a few small exceptions, to fix up inconsistencies, there are no changes to the code base and all previous code will run without change.
Because of the large number of files changed, this PR will probably be very difficult to review based on looking at the changes to individual files. It will probably make more sense to look through the updated version of the documentation on ReadTheDocs.
Summary of significant changes:
- Sphinx documentation is now divided into a User Guide, which provides a narrative style description of python-control package, and a Reference Manual, which has a listing of all functions, classes, package configuraton parameters, and other detailed information about the package.
- Control plots included in the documentation are generated by the code in the documentation, using the Sphinx doctest extension. Figures are created using the
make doctestcommand in thedoc/directory (called automatically whenmake htmlis run). - Regularized the way that classes, functions, methods, parameters, built-ins, and code samples are annotated and created a
custom.cssfile to control HTML formatting:- Classes, functions, methods, and parameters use single backticks, with no additional directives, and are treated using the
py:objdirective. For classes, functions, and methods, this will generate a link using bold, code font. For parameters, non-bold text is used (since Sphinx does not yet support links to parameter documentation). - Code samples use double backticks and are rendered in non-bold, black, fixed width font (versus the default red color).
- Python built-ins (True, False, None) are written with no backticks. This was previously very non-uniform, and this style fits what is commonly used in the NumPy documentation.
- Classes, functions, methods, and parameters use single backticks, with no additional directives, and are treated using the
- Moved the Control Systems Classes chapter to the Reference Manual and added all classes in the package (previous it was just the I/O system classes).
- Added a new chapter Package Configuration Parameters to the Reference Manual, with documentation on everything available via
ct.config.defaults(and a unit test to make sure nothing is missing). - Added a new chapter Developer Notes to the reference manual that describes the various choices that were made in making the package structure and documentation uniform. Sections in the chapter:
- Package Structure: how the package is organized (directories, files)
- Naming Conventions: how to name files, classes, functions, etc
- Documentation Guidelines: how to document various types of objects, including consistent use of backticks (with rationale), as well a description of what goes in the User Guide vs Reference Manual chapters.
- Utility Functions: a relatively incomplete listing of some of the more common utility functions for developers.
- Sample Files:
template.pyandtemplate.rstthat implement the guidelines.
- Got rid of docstring hashes for functions with variable arguments and instead used docstring signatures as a replacement for checking that parameters are documented.
- Added
numpydocchecks incontrol/tests/docstring_test.pyto pick up things like improperly labeled sections (e.g., "See also" instead of "See Also") and other errors. - Moved documentation from
__init__docstrings (which is not included in the Sphinx documentation) to class documentation (with details in the factory function, when appropriate). - Moved documentation examples files to
doc/examplesand documentation figures todoc/figuresto declutter thedocdirectory (this accounts for many of the 226 files that have changed). - [26 Jan 2025] Added Release Notes providing a (user-oriented) summary of changes in recent releases (with a summary of earlier releases). Release notes are contained in
doc/releases.
Smaller changes:
- Added Sphinx unit test (
doc/test_sphinx) to make sure all primary functions are documented in the Reference Manual. - Updated file header information to be a standard form, shown in
examples/template.py. - Removed top-level class dependence on
object - Removed (unused) table in
matlab/__init__and replaced withdoc/matlab.rst(part of Reference Manual) - Ran
isort -m2on all files to sort imports. - Improved consistency checking in
control/tests/docstring_test.pyunit test checks:- All function and parameter summaries now start with a capital letter and end with a period.
- Added checking docstring format of "Returns" section, in addition to "Parameters".
- Remove excess spaces throughout the package (since all files were touched).
- Equations are now centered in Sphinx, instead of left justified.
Small code changes:
- Renamed
ackertoplace_acker(to be consistent withplace_varga) and setacker = place_acker. - Identified source code lines that were more than 79 characters and reformatted (PEP 8).
- [25 Jan 2025] The
NamedSignalclass now has a__repr__method that evaluates back to aNamedSignal(similar to theInputOutputSystem__repr__method).
Additional changes that are coming:
- Regularize frequency response arguments/return vals: (fresp, freqpts)
- Regularize timebase documentation ('dt' format + wording)
- Make sure all MATLAB functions are documented in Reference Manual
- Get rid of numbered notes (just use paragraphs)
- Remove all remaining .. todo::'s
- Fix additional typos and grammatical inconsistencies
murrayrm
marked this pull request as ready for review
| -------- | ||
| freqresp | ||
| bode | ||
| frequency_response, bode_plot |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible to include Statespace.__call__and TransferFunction.__call__ here, and also in evalfr below? (Is there also a __call__ for frd systems? )
also - my opinion is that evalfr should be deprecated and only left in the Matlab module. Or given a more pythonic name that serves as a wrapper for call. But in any case, evalfr appears nowhere else in the documentation and should not be referenced in frequency_response. but __call__ should be.
| ss | ||
| ss2tf | ||
| tf2ss | ||
| TransferFunction, ss, ss2tf, tf2ss |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nlsys
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this, and I don't think we need to reference nlsys here. It is another factory function, but not that directly relevant to transfer functions. I will include nlsys in the ss factory function docstring.
| tf | ||
| ss | ||
| tf2ss | ||
| tf, ss, tf2ss |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nlsys
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, will leave nlsys off here.
| raise NotImplementedError("dcgain not implemented for %s objects" % | ||
| str(self.__class__)) | ||
| """Return the zero-frequency (DC) gain.""" | ||
| return NotImplemented |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| phase_crossover_frequencies | ||
| singular_values_response | ||
| tfdata | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Margin
| margins as well as the frequencies at which they occur:: | ||
|
|
||
| >>> sys = ct.tf(10, [1, 2, 3, 4]) | ||
| >>> gm, pm, sm, wpc, wgc, wms = ct.stability(sys) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stability_margins?
Thanks for the comments @sawyerbfuller. I'll update these in the next push.
I'm not sure what's up with the failing tests. It looks like there is a (non-transient) error in Netscape Portable Runtime (NSPR).
Great for newcomers that you’re working on this docs cleanup, I am in favor of the changes you’re working on.
I’m not an expert in colab but I have often just run it with a !pip install control and it seems to intelligently cache and not re-download the package every time. Are you sure that the whole
try: import control as ct print("python-control", ct.__version__) except ImportError: !pip install control import control as ct
is necessary in the intro sections? Apologies for bad formatting, I’m typing this on my phone)
I’m not sure whether the introductory tutorial is new or not but it is certainly more discoverable now. Nicely showcases the new time and frequency response plotting facilities. A few notes:
- could save an import using np.pi instead of math.pi
- Reveals a paper cut in
stepinfoin that many/all of its results are in singleton incarnations of various object types (eg np.float64) rather than plain floats.
I’m not an expert in colab but I have often just run it with a
!pip install controland it seems to intelligently cache and not re-download the package every time. Are you sure that the wholetry: import control as ct print("python-control", ct.__version__) except ImportError: !pip install control import control as ctis necessary in the intro sections? Apologies for bad formatting, I’m typing this on my phone)
I think you are right that Colab won't re-download the package, but the try/except may still be needed if you want the notebook to work in Jupyter. I'll go back and check and update in the next code push if it isn't needed.
In part 3,
- in the TF section, specify that num and den should be arrays of coefficients ordered in decreasing power of s (or z in discrete-time case)
- in the FRD section, give the units of omega (rad/s) and a little while later examples are specified in terms of freqpts instead, and it’s not clear what the relationship is. Maybe can just use omega in place of freqpts?
- “.. doctest..” appears in a box after “ A loadable description of a system can be obtained just by displaying the system object:” in sec. 3.6.
- clicking “next” at the end of 7.4 takes you to a stochastic systems example instead of taking you to chapter 8.
Add sample_system to the listing of “ Functions that transform systems from one form to another:” in “function reference”
I’m not an expert in colab but I have often just run it with a
!pip install controland it seems to intelligently cache and not re-download the package every time. Are you sure that the wholetry: import control as ct print("python-control", ct.__version__) except ImportError: !pip install control import control as ct
is necessary in the intro sections? Apologies for bad formatting, I’m typing this on my phone)I think you are right that Colab won't re-download the package, but the try/except may still be needed if you want the notebook to work in Jupyter. I'll go back and check and update in the next code push if it isn't needed.
Confirmed that !pip in Colab will not reinstall if it is already there => OK to leave out the try statement. I'm going to go ahead and leave that in for the tutorial since !pip may not run in all environments (eg, it fails in "raw" python).
I’m not sure whether the introductory tutorial is new or not but it is certainly more discoverable now. Nicely showcases the new time and frequency response plotting facilities. A few notes:
- could save an import using np.pi instead of math.pi
- Reveals a paper cut in
stepinfoin that many/all of its results are in singleton incarnations of various object types (eg np.float64) rather than plain floats.
I'm updating step_info to save everything as a float. This is a problem a bit more generally, since lots of things return np.float64 objects.
murrayrm
marked this pull request as ready for review
| @@ -1079,14 +1046,14 @@ def ctrb(A, B, t=None): | |||
| Parameters | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Controllabilty" at the top of this docstring is now fixed on main branch via #1107
Comment on lines +597 to +602
| try: | ||
| result = (sys**-1 * sys).minreal() | ||
| expected = StateSpace([], [], [], np.eye(2), dt=0) | ||
| assert _tf_close_coeff( | ||
| ss2tf(expected).minreal(), | ||
| ss2tf(result).minreal(), | ||
| ) | ||
| except AssertionError: | ||
| if platform.system() == 'Darwin': | ||
| pytest.xfail("minreal bug on MacOS") | ||
| else: | ||
| raise |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll plan to merge #1109 first, then rebase and remove this exception.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters