Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
murrayrm merged 6 commits intopython-control:mainfromsawyerbfuller:update-docs
Dec 17, 2022

Conversation

sawyerbfuller
Copy link
Contributor

@sawyerbfullersawyerbfuller commentedNov 30, 2022
edited
Loading

Improvements needed when looking at the html function reference

  • addNonlinearIOSystem to system creation section,interconnect to interconnection section
  • removeevalfr 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.
  • updateslyap andcare descriptions to be consistent with other functions
  • misc other formatting and consistency issues in various functions

…istent with other functions, fix formatting in a few other functions. add references to NonlinearIOSystem and __call__ and remove evalfr
@coveralls
Copy link

coveralls commentedNov 30, 2022
edited
Loading

Coverage Status

Coverage increased (+0.007%) to 94.838% when pulling16457d7 on sawyerbfuller:update-docs into8c764d0 on python-control:main.

@sawyerbfuller
Copy link
ContributorAuthor

old:
image

image

new:
image

image

Copy link
Member

@murrayrmmurrayrm left a 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.

Comment on lines 97 to 101
Returns
-------
sysd : linsys
Discrete time system, with sampling rate Ts

Copy link
Member

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:

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

Copy link
ContributorAuthor

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"

Comment on lines 83 to 84
TransferFunction.__call__
StateSpace.__call__
Copy link
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 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.

Copy link
ContributorAuthor

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

Copy link
ContributorAuthor

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.

h2syn
hinfsyn
lqr
dlqr
Copy link
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 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.

Copy link
ContributorAuthor

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.

Copy link
Member

@murrayrmmurrayrm left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -84,6 +84,7 @@
from ..rlocus import rlocus
from ..dtime import c2d
from ..sisotool import sisotool
from ..stochsys import *
Copy link
Contributor

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?

Copy link
Member

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.

bnavigator reacted with thumbs up emoji
@murrayrmmurrayrm merged commitb4cd96b intopython-control:mainDec 17, 2022
@murrayrmmurrayrm mentioned this pull requestDec 17, 2022
@murrayrmmurrayrm added this to the0.9.3 milestoneDec 24, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@murrayrmmurrayrmmurrayrm left review comments

@bnavigatorbnavigatorbnavigator left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9.3
Development

Successfully merging this pull request may close these issues.

4 participants
@sawyerbfuller@coveralls@murrayrm@bnavigator

[8]ページ先頭

©2009-2025 Movatter.jp