Feature enable doctest #868

henklaak · 2023-02-21T12:20:47Z

Hi, PR to enable doctest

Introducing a new doc/Makefile target: 'doctest'
Run doctests manually with:

cd doc
make html doctest

Introducing a new CI workflow: 'doctest'
This will run doctest on ubuntu-latest, python 3.11 with full package dependencies.
This one configuration is sufficient and required for running all doctests.

Reworked and added many docstrings.

coveralls · 2023-02-21T12:33:15Z

Coverage: 94.866% (+0.02%) from 94.841% when pulling df38286 on henklaak:feature_enable_doctest into 26c44e1 on python-control:main.

henklaak · 2023-02-21T12:40:46Z

Oh, one possibly contentious thingy.
The Doctest workflow generates a test artifact doctest-output which contains the test results.
I thought it might come in handy at times.

murrayrm

Great contribution to the library. I had a lot of small comments as I read through. A few more substantive ones:

I didn't understand the reason for changing classes.pdf to classes.png. The former should display with much better resolution.
I don't think we need to include check_{cvxopt,pandas,slycot} in the sphinx docs nor include examples for them. They are essentially internal functions.
In some places the outputs that are shown are not very informative (eg, the dimensions of a system rather than the system in pade).
If we are going to update docstrings, we should update the examples to match the more modern usage (eg, using ss([[1, 2], [3, 4]], [[5], [6]], [[7, 8]], [[9]]) instead of the MATLAB-esque ss("1 2; 3 4", "5; 6", "7 8", "9").

murrayrm · 2023-03-20T04:00:54Z

control/canonical.py

+    matrix([[1.],
+            [0.]])


Shouldn't this be array instead of matrix? And why skip the doctest?

This same issue come up multiple times below.

I fixed these in 98fdb35. In a few cases I had to use round() to get rid of problems between platforms (see 1d1c289), and for one or two cases I had to skip the test because 0. would become -0. on some platforms, even after rounding.

Shouldn't this be array instead of matrix? And why skip the doctest?

This same issue come up multiple times below.

Doctest has an existing flaw/feature, in that it doesn't do float comparisons. Down below, it's just string compare.
When system precision affects the result, small tolerances can lead to test failure.
I'll have a hard look at it again, because it annoys me no end.
(Might even have to submit a patch to the Doctest project ;-)

All the round() and other tricks to get around it are just confusing for users who look at the examples.

Good point. I have removed this and will go through an +SKIP any that fail.

murrayrm · 2023-03-20T04:03:15Z

control/canonical.py

+    >>> G = tf([1],[1, 3, 2])
+    >>> Gs = tf2ss(G)


You could simplify this to be G = tf2ss([1], [1, 3, 2]).

I fixed all of these in 98fdb35.

I tried to keep the examples very much step-by-step, so users can understand things more easily.
I'm perfectly fine with anything that teaches the user better habits.

murrayrm · 2023-03-20T04:05:36Z

control/config.py

+    >>> # do some customized freqplotting
+    >>> reset_defaults()
+    >>> defaults['freqplot.number_of_samples']
+    1000


Does this belong here? It is repeated in reset_defaults, below.

I got rid of line 82, but line 81 is required so that doctest leaves the state unchanged for the next test in this module.

murrayrm · 2023-03-20T04:08:42Z

control/ctrlutil.py

-    >>> unwrap(theta, period=2 * np.pi)
-    [5.74, 5.97, 6.19, 6.413185307179586, 6.633185307179586, 6.8531853071795865]
+    >>> from control import unwrap
+    >>> from pprint import pprint


Is this used?

murrayrm · 2023-03-20T04:10:30Z

control/ctrlutil.py

+
+    Examples
+    --------
+    >>> from control import issys, tf, InputOutputSystem, LinearIOSystem


LinearIOSystem is not used and InputOutputSystem can probably be replaced with something else (see below).

murrayrm · 2023-03-20T04:55:38Z

doc/Makefile

-classes.pdf: classes.fig;	fig2dev -Lpdf $< $@
+FIGS = classes.png
+classes.png: classes.fig
+	fig2dev -Lpng $< $@


Why change from pdf to png? The pdf will render much more nicely? Also, if there is a reason to keep the PNG file, then we should not add it to the repository (it is created by the Makefile).

I reverted this change.

murrayrm · 2023-03-20T04:55:59Z

doc/classes.rst

@@ -24,7 +24,7 @@ The following figure illustrates the relationship between the classes and
 some of the functions that can be used to convert objects from one class to
 another:

-.. image:: classes.pdf
+.. image:: classes.png


Suggest leaving as PDF unless there is a reason not to.

murrayrm · 2023-03-20T04:56:36Z

doc/control.rst

@@ -172,6 +172,7 @@ Utility functions and conversions
    augw
    bdschur
    canonical_form
+    cvxopt_check


Not really intended as a user function; we should probably not include it here.

I got rid of the *_check entries.

murrayrm · 2023-03-20T04:56:41Z

doc/control.rst

@@ -182,9 +183,13 @@ Utility functions and conversions
    modal_form
    observable_form
    pade
+    pandas_check


Not really intended as a user function; we should probably not include it here.

murrayrm · 2023-03-20T04:56:51Z

doc/control.rst

    reachable_form
    reset_defaults
    sample_system
+    set_defaults
+    similarity_transform
+    slycot_check


Not really intended as a user function; we should probably not include it here.

murrayrm · 2023-03-25T15:09:51Z

@henklaak I went ahead and made updates to resolve most of my comments. In addition to the addressing substantive comments in my overall review, I also set up doctests to use the ct prefix for all python-control functions, which matches the way NumPy docstring examples are done. The import command that sets this up is in doc/conf.py (it also imports numpy as np).

I'll leave this PR sitting for a few days in case you want to make comments, but I think it is ready to be merged. Thanks again for this very nice contribution!

murrayrm requested changes Mar 20, 2023

View reviewed changes

henklaak and others added 6 commits March 24, 2023 22:47

Enable doctest

562701c

Docs update

c913c8f

remove imports and use ct.fcn instead

c4c275f

clean up/simplify docstring examples

98fdb35

revert to use classes.pdf in doc/ (instead of classes.png)

07cedd8

fix up rounding errors and matrix/array

1d1c289

murrayrm approved these changes Mar 25, 2023

View reviewed changes

murrayrm added 4 commits March 25, 2023 15:58

Merge branch 'main' into feature_enable_doctest

927f478

fix up errors from merge with zpk feature

c39bc87

revert use of round() in canonical examples (fix doctest failures next)

983cbc0

skip doctests with rounding errors in GitHub Actions

df38286

murrayrm merged commit 2c5c2f0 into python-control:main Mar 27, 2023

murrayrm added this to the 0.9.4 milestone Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature enable doctest #868

Feature enable doctest #868

henklaak commented Feb 21, 2023 •

edited

Loading

coveralls commented Feb 21, 2023 •

edited

Loading

henklaak commented Feb 21, 2023 •

edited

Loading

murrayrm left a comment

murrayrm Mar 20, 2023

murrayrm Mar 25, 2023

henklaak Mar 27, 2023 •

edited

Loading

murrayrm Mar 27, 2023

murrayrm Mar 20, 2023

murrayrm Mar 25, 2023

henklaak Mar 27, 2023

murrayrm Mar 20, 2023

murrayrm Mar 25, 2023

murrayrm Mar 20, 2023

murrayrm Mar 20, 2023

murrayrm Mar 20, 2023

murrayrm Mar 25, 2023

murrayrm Mar 20, 2023

murrayrm Mar 20, 2023

murrayrm Mar 25, 2023

murrayrm Mar 20, 2023

murrayrm Mar 20, 2023

murrayrm commented Mar 25, 2023

Feature enable doctest #868

Feature enable doctest #868

Conversation

henklaak commented Feb 21, 2023 • edited Loading

coveralls commented Feb 21, 2023 • edited Loading

henklaak commented Feb 21, 2023 • edited Loading

murrayrm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henklaak Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murrayrm commented Mar 25, 2023

henklaak commented Feb 21, 2023 •

edited

Loading

coveralls commented Feb 21, 2023 •

edited

Loading

henklaak commented Feb 21, 2023 •

edited

Loading

henklaak Mar 27, 2023 •

edited

Loading