- Notifications
You must be signed in to change notification settings - Fork441
docstring improvements#804
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
…istent with other functions, fix formatting in a few other functions. add references to NonlinearIOSystem and __call__ and remove evalfr
coveralls commentedNov 30, 2022 • 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.
…r appear sequentially
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 catches on the documentation. I think we should comply with thenumpydoc style guide, which involves renaming "Additional Parameters" to "Other Parameters" and leaving themafter the "Returns" section.
control/dtime.py Outdated
Returns | ||
------- | ||
sysd : linsys | ||
Discrete time system, with sampling rate Ts | ||
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.
Per thenumpydoc style guide, the "Other Parameters" section should comeafter the "Returns" section. When processed using numpydoc (eg, for readthedocs), these parameters will get lumped with all other parameters in a single section. But when they are displayed in an interactive session, they are shown in the order listed (so you see the most common parameters, then what the function returns, then other parameters).
For an example, seect.input_output_response
. If you look at the docstring in an interactive session (eg,?ct.input_output_response
in iPython) then you see the the documentation forsolve_ivp_method
(a rarely used parameter) after the "Returns" section:
Compute the output response of a system to a given input. Simulate a dynamical system with a given input and return its output and state values. Parameters ---------- sys : InputOutputSystem Input/output system to simulate. T : array-like Time steps at which the input is defined; values must be evenly spaced. U : array-like, list, or number, optional Input array giving input at each time `T` (default = 0). If a list is specified, each element in the list will be treated as a portion of the input and broadcast (if necessary) to match the time vector. X0 : array-like, list, or number, optional Initial condition (default = 0). If a list is given, each element in the list will be flattened and stacked into the initial condition. If a smaller number of elements are given that the number of states in the system, the initial condition will be padded with zeros. return_x : bool, optional If True, return the state vector when assigning to a tuple (default = False). See :func:`forced_response` for more details. If True, return the values of the state at each time (default = False). squeeze : bool, optional If True and if the system has a single output, return the system output as a 1D array rather than a 2D array. If False, return the system output as a 2D array even if the system is SISO. Default value set by config.defaults['control.squeeze_time_response']. Returns ------- results : TimeResponseData Time response represented as a :class:`TimeResponseData` object containing the following properties: * time (array): Time values of the output. * outputs (array): Response of the system. If the system is SISO and `squeeze` is not True, the array is 1D (indexed by time). If the system is not SISO or `squeeze` is False, the array is 2D (indexed by output and time). * states (array): Time evolution of the state vector, represented as a 2D array indexed by state and time. * inputs (array): Input(s) to the system, indexed by input and time. The return value of the system can also be accessed by assigning the function to a tuple of length 2 (time, output) or of length 3 (time, output, state) if ``return_x`` is ``True``. If the input/output system signals are named, these names will be used as labels for the time response. Other parameters ---------------- solve_ivp_method : str, optional Set the method used by :func:`scipy.integrate.solve_ivp`. Defaults to 'RK45'. solve_ivp_kwargs : dict, optional Pass additional keywords to :func:`scipy.integrate.solve_ivp`. Raises ------ TypeError If the system is not an input/output system. ValueError If time step does not match sampling time (for discrete time systems). Notes ----- 1. If a smaller number of initial conditions are given than the number of states in the system, the initial conditions will be padded with zeros. This is often useful for interconnected control systems where the process dynamics are the first system and all other components start with zero initial condition since this can be specified as [xsys_0, 0]. A warning is issued if the initial conditions are padded and and the final listed initial state is not zero.
But if you look at the documentation onreadthedocs, you see all of the parameters gathered together:
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.
Thanks. I had noticed that there were a few places where the docstrings in the library listed "additional Parameters" and sphinx was not handling it right. So I took it out. And since making this commit I discovered that sphinx was looking for "other parameters." I'll look around for any more instances of "additional parameters" and change them back to "other parameters"
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
doc/control.rst Outdated
TransferFunction.__call__ | ||
StateSpace.__call__ |
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 agree this format is a bit long and cumbersome. Would it make sense to have this show up as something more likesys(x[, squeeze])
or something like that. This can be done by writing the desired signature as the first line of the docstring, as describedhere.
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.
Ok looked around in numpy and scipy and foundscipy.interpolate.interp1d
, which is a class that has a__call__
method. But it is not linked in a function reference like this, so it did not provide a way to change how the call signature is shown. Perhaps there is a way to tell sphinx to manually change its call signature, but I coudln't find it.
Unless anybody else has other ideas about how to refer to this, I would propose to leave it as it is in this PR and plan to update later if somebody figures something out.
doc/control.rst Outdated
h2syn | ||
hinfsyn | ||
lqr | ||
dlqr |
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 understand why this might go here, but doesn't it make more sense to list alphabetically?
Alternatively, we could list this aslqe, dlqe
, since they go together?
Note that if you pass aLTI
object tolqr
(andlqe
), the function will check the timebase to determine whether to use the continuous time or discrete time version of the function.
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.
re-alphebetized them in recent commit.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM
control/matlab/__init__.py Outdated
@@ -84,6 +84,7 @@ | |||
from ..rlocus import rlocus | |||
from ..dtime import c2d | |||
from ..sisotool import sisotool | |||
from ..stochsys import * |
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.
Can we make these explicit imports please?
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 think the two functions that we want (since they are present in MATLAB) are lqe and dlqe. The other functions are unique to python-control.
Uh oh!
There was an error while loading.Please reload this page.
Improvements needed when looking at the html function reference
NonlinearIOSystem
to system creation section,interconnect
to interconnection sectionevalfr
andfreqresp
(they still live in matlab module). Replace them with__call__
methods andfrequency_response
respectively. The former ends up being a bit verbose, I'm open to other ideas for how to present this.lyap
andcare
descriptions to be consistent with other functions