- Notifications
You must be signed in to change notification settings - Fork441
Feature enable doctest#868
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedFeb 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
henklaak commentedFeb 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Oh, one possibly contentious thingy. |
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.
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.pdftoclasses.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-esquess("1 2; 3 4", "5; 6", "7 8", "9").
control/canonical.py Outdated
| matrix([[1.], | ||
| [0.]]) |
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.
Shouldn't this bearray instead ofmatrix? And why skip the doctest?
This same issue come up multiple times below.
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.
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.
Shouldn't this be
arrayinstead ofmatrix? 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.
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.
Good point. I have removed this and will go through an +SKIP any that fail.
control/canonical.py Outdated
| >>> G = tf([1],[1, 3, 2]) | ||
| >>> Gs = tf2ss(G) |
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.
You could simplify this to beG = tf2ss([1], [1, 3, 2]).
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 fixed all of these in98fdb35.
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 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.
control/config.py Outdated
| >>> # do some customized freqplotting | ||
| >>> reset_defaults() | ||
| >>> defaults['freqplot.number_of_samples'] | ||
| 1000 |
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.
Does this belong here? It is repeated inreset_defaults, below.
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 got rid of line 82, but line 81 is required so that doctest leaves the state unchanged for the next test in this module.
control/ctrlutil.py Outdated
| >>> 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 |
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.
Is this used?
control/ctrlutil.py Outdated
| Examples | ||
| -------- | ||
| >>> from control import issys, tf, InputOutputSystem, LinearIOSystem |
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.
LinearIOSystem is not used andInputOutputSystem can probably be replaced with something else (see below).
doc/Makefile Outdated
| classes.pdf: classes.fig;fig2dev -Lpdf $< $@ | ||
| FIGS = classes.png | ||
| classes.png: classes.fig | ||
| fig2dev -Lpng $< $@ |
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.
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).
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 reverted this change.
doc/classes.rst Outdated
| @@ -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 | |||
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.
Suggest leaving as PDF unless there is a reason not to.
doc/control.rst Outdated
| @@ -172,6 +172,7 @@ Utility functions and conversions | |||
| augw | |||
| bdschur | |||
| canonical_form | |||
| cvxopt_check | |||
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.
Not really intended as a user function; we should probably not include it here.
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 got rid of the*_check entries.
doc/control.rst Outdated
| @@ -182,9 +183,13 @@ Utility functions and conversions | |||
| modal_form | |||
| observable_form | |||
| pade | |||
| pandas_check | |||
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.
Not really intended as a user function; we should probably not include it here.
doc/control.rst Outdated
| reachable_form | ||
| reset_defaults | ||
| sample_system | ||
| set_defaults | ||
| similarity_transform | ||
| slycot_check |
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.
Not really intended as a user function; we should probably not include it here.
@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 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! |
Uh oh!
There was an error while loading.Please reload this page.
Hi, PR to enable doctest
Introducing a new doc/Makefile target: 'doctest'
Run doctests manually with:
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.