GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — docstring improvements by sawyerbfuller · Pull Request #804 · python-control/python-control Conversation
|
|
Coverage increased (+0.007%) to 94.838% when pulling 16457d7 on sawyerbfuller:update-docs into 8c764d0 on python-control:main. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…r appear sequentially
There was a problem 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 the numpydoc style guide, which involves renaming "Additional Parameters" to "Other Parameters" and leaving them after the "Returns" section.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Returns | ||
| ------- | ||
| sysd : linsys | ||
| Discrete time system, with sampling rate Ts | ||
|
|
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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"
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| TransferFunction.__call__ | ||
| StateSpace.__call__ |
There was a problem 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 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| h2syn | ||
| hinfsyn | ||
| lqr | ||
| dlqr |
There was a problem 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 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
re-alphebetized them in recent commit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| from ..rlocus import rlocus | ||
| from ..dtime import c2d | ||
| from ..sisotool import sisotool | ||
| from ..stochsys import * |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can we make these explicit imports please?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.