Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
murrayrm merged 6 commits into from
Dec 17, 2022
Merged

docstring improvements #804

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3a554b5
docstring fixes: standardize description for lyap and care to be cons…
sawyerbfuller Nov 30, 2022
11d9350
a few more fixes to sys.sample
sawyerbfuller Nov 30, 2022
bf50ded
mention lqe and dlqe in matlab module, reorder so that eg lqr and dlq…
sawyerbfuller Dec 2, 2022
2311a4d
bring back 'Other parameters' in docstrings, alphabetize TOC
sawyerbfuller Dec 12, 2022
7acd3fc
added signature for drss, import missing stochsys for matlab module, …
sawyerbfuller Dec 12, 2022
16457d7
explicit import of stochsys in matlab, per @bnavigator
murrayrm Dec 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions control/dtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,24 +84,21 @@ def sample_system(sysc, Ts, method='zoh', alpha=None, prewarp_frequency=None,
copy_names : bool, Optional
If True, copy the names of the input signals, output
signals, and states to the sampled system.

Returns
-------
sysd : linsys
Discrete time system, with sampling rate Ts

Additional Parameters
---------------------
inputs : int, list of str or None, optional
Description of the system inputs. If not specified, the origional
system inputs are used. See :class:`NamedIOSystem` for more
system inputs are used. See :class:`InputOutputSystem` for more
information.
outputs : int, list of str or None, optional
Description of the system outputs. Same format as `inputs`.
states : int, list of str, or None, optional
Description of the system states. Same format as `inputs`. Only
available if the system is :class:`StateSpace`.

Returns
-------
sysd : linsys
Discrete time system, with sampling rate Ts

@murrayrm murrayrm Dec 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for 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:

Screen Shot 2022-12-11 at 9 01 28 AM

@sawyerbfuller sawyerbfuller Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

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"

Notes
-----
See :meth:`StateSpace.sample` or :meth:`TransferFunction.sample` for
Expand Down
21 changes: 9 additions & 12 deletions control/iosys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2203,24 +2203,21 @@ def linearize(sys, xeq, ueq=None, t=0, params=None, **kw):
copy_names : bool, Optional
If True, Copy the names of the input signals, output signals, and
states to the linearized system.

Returns
-------
ss_sys : LinearIOSystem
The linearization of the system, as a :class:`~control.LinearIOSystem`
object (which is also a :class:`~control.StateSpace` object.

Additional Parameters
---------------------
Comment thread
sawyerbfuller marked this conversation as resolved.
Resolved
inputs : int, list of str or None, optional
Description of the system inputs. If not specified, the origional
system inputs are used. See :class:`NamedIOSystem` for more
system inputs are used. See :class:`InputOutputSystem` for more
information.
outputs : int, list of str or None, optional
Description of the system outputs. Same format as `inputs`.
states : int, list of str, or None, optional
Description of the system states. Same format as `inputs`.

Returns
-------
ss_sys : LinearIOSystem
The linearization of the system, as a :class:`~control.LinearIOSystem`
object (which is also a :class:`~control.StateSpace` object.

"""
if not isinstance(sys, InputOutputSystem):
raise TypeError("Can only linearize InputOutputSystem types")
Expand Down Expand Up @@ -2716,8 +2713,8 @@ def interconnect(syslist, connections=None, inplist=None, outlist=None,
:func:`~control.summing_block` function and the ability to automatically
interconnect signals with the same names:

>>> P = control.tf2io(control.tf(1, [1, 0]), inputs='u', outputs='y')
>>> C = control.tf2io(control.tf(10, [1, 1]), inputs='e', outputs='u')
>>> P = control.tf(1, [1, 0], inputs='u', outputs='y')
>>> C = control.tf(10, [1, 1], inputs='e', outputs='u')
>>> sumblk = control.summing_junction(inputs=['r', '-y'], output='e')
>>> T = control.interconnect([P, C, sumblk], inputs='r', outputs='y')

Expand Down
17 changes: 12 additions & 5 deletions control/mateqn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ def sb03md(n, C, A, U, dico, job='X', fact='N', trana='N', ldwork=None):


def lyap(A, Q, C=None, E=None, method=None):
"""X = lyap(A, Q) solves the continuous-time Lyapunov equation
"""Solves the continuous-time Lyapunov equation

X = lyap(A, Q) solves

:math:`A X + X A^T + Q = 0`

Expand Down Expand Up @@ -217,7 +219,9 @@ def lyap(A, Q, C=None, E=None, method=None):


def dlyap(A, Q, C=None, E=None, method=None):
"""dlyap(A, Q) solves the discrete-time Lyapunov equation
"""Solves the discrete-time Lyapunov equation

X = dlyap(A, Q) solves

:math:`A X A^T - X + Q = 0`

Expand Down Expand Up @@ -348,8 +352,9 @@ def dlyap(A, Q, C=None, E=None, method=None):

def care(A, B, Q, R=None, S=None, E=None, stabilizing=True, method=None,
A_s="A", B_s="B", Q_s="Q", R_s="R", S_s="S", E_s="E"):
"""X, L, G = care(A, B, Q, R=None) solves the continuous-time
algebraic Riccati equation
"""Solves the continuous-time algebraic Riccati equation

X, L, G = care(A, B, Q, R=None) solves

:math:`A^T X + X A - X B R^{-1} B^T X + Q = 0`

Expand Down Expand Up @@ -505,9 +510,11 @@ def care(A, B, Q, R=None, S=None, E=None, stabilizing=True, method=None,

def dare(A, B, Q, R, S=None, E=None, stabilizing=True, method=None,
A_s="A", B_s="B", Q_s="Q", R_s="R", S_s="S", E_s="E"):
"""X, L, G = dare(A, B, Q, R) solves the discrete-time algebraic Riccati
"""Solves the discrete-time algebraic Riccati
equation

X, L, G = dare(A, B, Q, R) solves

:math:`A^T X A - X - A^T X B (B^T X B + R)^{-1} B^T X A + Q = 0`

where A and Q are square matrices of the same dimension. Further, Q
Expand Down
8 changes: 4 additions & 4 deletions control/sisotool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,6 @@ def rootlocus_pid_designer(plant, gain='P', sign=+1, input_signal='r',

C_f = Kp + Ki/s + Kd*s/(tau*s + 1).

If `plant` is a discrete-time system, then the proportional, integral, and
derivative terms are given instead by Kp, Ki*dt/2*(z+1)/(z-1), and
Kd/dt*(z-1)/z, respectively.

::

------> C_ff ------ d
Expand All @@ -224,6 +220,10 @@ def rootlocus_pid_designer(plant, gain='P', sign=+1, input_signal='r',
| ----- C_b <-------|
---------------------------------

If `plant` is a discrete-time system, then the proportional, integral, and
derivative terms are given instead by Kp, Ki*dt/2*(z+1)/(z-1), and
Kd/dt*(z-1)/z, respectively.

It is also possible to move the derivative term into the feedback path
`C_b` using `derivative_in_feedback_path=True`. This may be desired to
avoid that the plant is subject to an impulse function when the reference
Expand Down
16 changes: 7 additions & 9 deletions control/statesp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ def __rdiv__(self, other):
"StateSpace.__rdiv__ is not implemented yet.")

def __call__(self, x, squeeze=None, warn_infinite=True):
"""Evaluate system's transfer function at complex frequency.
"""Evaluate system's frequency response at complex frequencies.

Returns the complex frequency response `sys(x)` where `x` is `s` for
continuous-time systems and `z` for discrete-time systems.
Expand Down Expand Up @@ -1338,14 +1338,6 @@ def sample(self, Ts, method='zoh', alpha=None, prewarp_frequency=None,
copy_names : bool, Optional
If True, copy the names of the input signals, output
signals, and states to the sampled system.

Returns
-------
sysd : StateSpace
Discrete-time system, with sampling rate Ts

Additional Parameters
---------------------
Comment thread
sawyerbfuller marked this conversation as resolved.
Resolved
inputs : int, list of str or None, optional
Description of the system inputs. If not specified, the origional
system inputs are used. See :class:`InputOutputSystem` for more
Expand All @@ -1355,6 +1347,11 @@ def sample(self, Ts, method='zoh', alpha=None, prewarp_frequency=None,
states : int, list of str, or None, optional
Description of the system states. Same format as `inputs`.

Returns
-------
sysd : StateSpace
Discrete-time system, with sampling rate Ts

Notes
-----
Uses :func:`scipy.signal.cont2discrete`
Expand Down Expand Up @@ -1520,6 +1517,7 @@ def output(self, t, x, u=None, params=None):


# TODO: add discrete time check
# TODO: copy signal names
def _convert_to_statespace(sys):
"""Convert a system to state space form (if needed).

Expand Down
15 changes: 6 additions & 9 deletions control/xferfcn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,21 +1130,18 @@ def sample(self, Ts, method='zoh', alpha=None, prewarp_frequency=None,
copy_names : bool, Optional
If True, copy the names of the input signals, output
signals, and states to the sampled system.

Returns
-------
sysd : TransferFunction system
Discrete-time system, with sample period Ts

Additional Parameters
---------------------
Comment thread
sawyerbfuller marked this conversation as resolved.
Resolved
inputs : int, list of str or None, optional
Description of the system inputs. If not specified, the origional
system inputs are used. See :class:`NamedIOSystem` for more
system inputs are used. See :class:`InputOutputSystem` for more
information.
outputs : int, list of str or None, optional
Description of the system outputs. Same format as `inputs`.

Returns
-------
sysd : TransferFunction system
Discrete-time system, with sample period Ts

Notes
-----
1. Available only for SISO systems
Expand Down
17 changes: 10 additions & 7 deletions doc/control.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ System creation
frd
rss
drss
NonlinearIOSystem


System interconnections
=======================
Expand All @@ -32,9 +34,8 @@ System interconnections
negate
parallel
series
interconnect

See also the :ref:`iosys-module` module, which can be used to create and
interconnect nonlinear input/output systems.

Frequency domain plotting
=========================
Expand Down Expand Up @@ -78,8 +79,9 @@ Control system analysis

dcgain
describing_function
evalfr
freqresp
frequency_response
TransferFunction.__call__
StateSpace.__call__

@murrayrm murrayrm Dec 11, 2022

Copy link
Copy Markdown
Member

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 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.

@sawyerbfuller sawyerbfuller Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I tried to rename the call string for TransferFunction.__call__ in the docstring as suggested in numpydoc but no luck:

image

Will see if I can find any examples in numpy doc itself I can use

@sawyerbfuller sawyerbfuller Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

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 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.

get_input_ff_index
get_output_fb_index
ispassive
Expand Down Expand Up @@ -114,10 +116,10 @@ Control system synthesis

acker
create_statefbk_iosystem
dlqr
h2syn
hinfsyn
lqr
dlqr

@murrayrm murrayrm Dec 11, 2022

Copy link
Copy Markdown
Member

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 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.

@sawyerbfuller sawyerbfuller Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

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.

mixsyn
place
rootlocus_pid_designer
Expand All @@ -141,7 +143,6 @@ Nonlinear system support

describing_function
find_eqpt
interconnect
linearize
input_output_response
ss2io
Expand All @@ -156,8 +157,8 @@ Stochastic system support

correlation
create_estimator_iosystem
dlqe
Comment thread
sawyerbfuller marked this conversation as resolved.
Resolved
lqe
dlqe
white_noise

.. _utility-and-conversions:
Expand Down Expand Up @@ -193,3 +194,5 @@ Utility functions and conversions
use_fbs_defaults
use_matlab_defaults
use_numpy_matrix


3 changes: 3 additions & 0 deletions doc/matlab.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ Compensator design
sisotool
place
lqr
dlqr
lqe
dlqe
Comment thread
sawyerbfuller marked this conversation as resolved.
Resolved

State-space (SS) models
=======================
Expand Down