docstring improvements #804
Conversation
…istent with other functions, fix formatting in a few other functions. add references to NonlinearIOSystem and __call__ and remove evalfr
|
old:
new:
|
…r appear sequentially
There was a problem hiding this comment.
Good catches on the documentation. I think we should comply with the numpydoc style guide, which involves renaming "Additional Parameters" to "Other Parameters" and leaving them after the "Returns" section.
control/dtime.py
Outdated
| Returns | ||
| ------- | ||
| sysd : linsys | ||
| Discrete time system, with sampling rate Ts | ||
|
|
There was a problem hiding this comment.
Per the numpydoc style guide, the "Other Parameters" section should come after 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, see ct.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 for solve_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 on readthedocs, you see all of the parameters gathered together:
There was a problem hiding this comment.
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"
doc/control.rst
Outdated
| TransferFunction.__call__ | ||
| StateSpace.__call__ |
There was a problem hiding this comment.
I agree this format is a bit long and cumbersome. Would it make sense to have this show up as something more like sys(x[, squeeze]) or something like that. This can be done by writing the desired signature as the first line of the docstring, as described here.
There was a problem hiding this comment.
Good idea. I tried to rename the call string for TransferFunction.__call__ in the docstring as suggested in numpydoc but no luck:
Will see if I can find any examples in numpy doc itself I can use
There was a problem hiding this comment.
Ok looked around in numpy and scipy and found scipy.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.
I understand why this might go here, but doesn't it make more sense to list alphabetically?
Alternatively, we could list this as lqe, dlqe, since they go together?
Note that if you pass a LTI object to lqr (and lqe), 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.
re-alphebetized them in recent commit.
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.
Can we make these explicit imports please?
There was a problem hiding this comment.
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.
Improvements needed when looking at the html function reference
NonlinearIOSystemto system creation section,interconnectto interconnection sectionevalfrandfreqresp(they still live in matlab module). Replace them with__call__methods andfrequency_responserespectively. The former ends up being a bit verbose, I'm open to other ideas for how to present this.lyapandcaredescriptions to be consistent with other functions