Skip to content

Update docstrings for LTI class constructors #171

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 2, 2018

Conversation

murrayrm
Copy link
Member

This PR addresses the documentation issues raised in PR #163 related to how the StateSpace, TransferFunction, and FRD constructors are documented. In this PR, the first line of the docstring is modified for the each of the above classes, as well as their init methods and the corresponding ss, tf, and frd functions to have the most common call signature.

This PR leaves in place the copy constructor functionality and only modifies the documentation.

While I was at it, I also fixed some indentation problems and other issues that were causing warnings in sphinx.

@murrayrm
Copy link
Member Author

Added more information to main documentation on class constructors + updated docstrings for other functions with variable arguments. The justification for including them in this PR is that they are consistent with the initial changes and the comments in PR #163.


.. math::

G(s) = \frac{\text{num}(s)}{\text{den(s)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

in first case (s) is outside \text{}, in second case inside it.

Linear time invariant (LTI) systems are represented in python-control in
state space, transfer function, or frequency response data (FRD) form. Most
functions in the toolbox will operate on any of these data types and
functions for convering between between compatible types is provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

converting

@@ -338,7 +336,9 @@ def phase_crossover_frequencies(sys):


def margin(*args):
"""Calculate gain and phase margins and associated crossover frequencies
"""margin(sys)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just below (line 345) the name "sysdata" is used; perhaps that should be used here?

# Hack for sphinx.ext.autodoc: if numpy is a mock import, then numpy.pi
# will be assigned to _Mock() and this generates a type error
if not isinstance(pi, float):
pi = 3.14
Copy link
Contributor

Choose a reason for hiding this comment

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

how about "from math import pi" here? Or does that result in a mocked instance of some sort too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Much better fix since math is included in the base python distribution.

= \frac{a_0 s^n + a_1 s^{n-1} + \cdots + a_n}
{b_0 s^m + b_1 s^{m-1} + \cdots + b_m},

where n is generally greater than m (for a proper transfer function).
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe greater than or equal to?

@murrayrm murrayrm mentioned this pull request Dec 30, 2017
@murrayrm
Copy link
Member Author

Good feedback @roryyorke. Small updates pushed to address these.

@murrayrm murrayrm merged commit f890a88 into python-control:master Jan 2, 2018
@murrayrm murrayrm deleted the statesp_constructor_doc branch January 2, 2018 16:13
@murrayrm murrayrm mentioned this pull request Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants