-
Notifications
You must be signed in to change notification settings - Fork 447
Updated documentation #1094
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
Updated documentation #1094
Conversation
44e40f6 to
480cda7
Compare
control/lti.py
Outdated
| -------- | ||
| freqresp | ||
| bode | ||
| frequency_response, bode_plot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible to include Statespace.__call__and TransferFunction.__call__ here, and also in evalfr below? (Is there also a __call__ for frd systems? )
also - my opinion is that evalfr should be deprecated and only left in the Matlab module. Or given a more pythonic name that serves as a wrapper for call. But in any case, evalfr appears nowhere else in the documentation and should not be referenced in frequency_response. but __call__ should be.
| ss | ||
| ss2tf | ||
| tf2ss | ||
| TransferFunction, ss, ss2tf, tf2ss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nlsys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this, and I don't think we need to reference nlsys here. It is another factory function, but not that directly relevant to transfer functions. I will include nlsys in the ss factory function docstring.
| tf | ||
| ss | ||
| tf2ss | ||
| tf, ss, tf2ss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nlsys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, will leave nlsys off here.
control/lti.py
Outdated
| raise NotImplementedError("dcgain not implemented for %s objects" % | ||
| str(self.__class__)) | ||
| """Return the zero-frequency (DC) gain.""" | ||
| return NotImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://docs.python.org/3.12/library/exceptions.html#NotImplementedError , abstract methods should raise an error if they’re not overridden.
| phase_crossover_frequencies | ||
| singular_values_response | ||
| tfdata | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Margin
doc/xferfcn.rst
Outdated
| margins as well as the frequencies at which they occur:: | ||
|
|
||
| >>> sys = ct.tf(10, [1, 2, 3, 4]) | ||
| >>> gm, pm, sm, wpc, wgc, wms = ct.stability(sys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stability_margins?
|
Thanks for the comments @sawyerbfuller. I'll update these in the next push. I'm not sure what's up with the failing tests. It looks like there is a (non-transient) error in Netscape Portable Runtime (NSPR). |
d601d38 to
68835e4
Compare
|
Great for newcomers that you’re working on this docs cleanup, I am in favor of the changes you’re working on. I’m not an expert in colab but I have often just run it with a is necessary in the intro sections? Apologies for bad formatting, I’m typing this on my phone) |
|
I’m not sure whether the introductory tutorial is new or not but it is certainly more discoverable now. Nicely showcases the new time and frequency response plotting facilities. A few notes:
|
I think you are right that Colab won't re-download the package, but the try/except may still be needed if you want the notebook to work in Jupyter. I'll go back and check and update in the next code push if it isn't needed. |
|
“ State space models can be manipulated …” appears twice in part 3 |
|
In part 3,
|
|
Add |
Confirmed that |
I'm updating |
1fd18b4 to
258e28e
Compare
|
A few more things as I find them, concentrating on docs:
|
9dc0480 to
7270f63
Compare
| def ctrb(A, B, t=None): | ||
| """Controllabilty matrix. | ||
| Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Controllabilty" at the top of this docstring is now fixed on main branch via #1107
control/tests/statesp_test.py
Outdated
| try: | ||
| result = (sys**-1 * sys).minreal() | ||
| expected = StateSpace([], [], [], np.eye(2), dt=0) | ||
| assert _tf_close_coeff( | ||
| ss2tf(expected).minreal(), | ||
| ss2tf(result).minreal(), | ||
| ) | ||
| except AssertionError: | ||
| if platform.system() == 'Darwin': | ||
| pytest.xfail("minreal bug on MacOS") | ||
| else: | ||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll plan to merge #1109 first, then rebase and remove this exception.
8f3701d to
7da8c00
Compare
This (very large) PR contains a restructuring of the Sphinx documentation, including changes to improve uniformity of docstrings along with draft guidelines for developers. This is being posted initially as a draft PR so that people can start to have a look at the changes and provide feedback.
High level principles:
Because of the large number of files changed, this PR will probably be very difficult to review based on looking at the changes to individual files. It will probably make more sense to look through the updated version of the documentation on ReadTheDocs.
Summary of significant changes:
make doctestcommand in thedoc/directory (called automatically whenmake htmlis run).custom.cssfile to control HTML formatting:py:objdirective. For classes, functions, and methods, this will generate a link using bold, code font. For parameters, non-bold text is used (since Sphinx does not yet support links to parameter documentation).ct.config.defaults(and a unit test to make sure nothing is missing).template.pyandtemplate.rstthat implement the guidelines.numpydocchecks incontrol/tests/docstring_test.pyto pick up things like improperly labeled sections (e.g., "See also" instead of "See Also") and other errors.__init__docstrings (which is not included in the Sphinx documentation) to class documentation (with details in the factory function, when appropriate).doc/examplesand documentation figures todoc/figuresto declutter thedocdirectory (this accounts for many of the 226 files that have changed).doc/releases.Smaller changes:
doc/test_sphinx) to make sure all primary functions are documented in the Reference Manual.examples/template.py.objectmatlab/__init__and replaced withdoc/matlab.rst(part of Reference Manual)isort -m2on all files to sort imports.control/tests/docstring_test.pyunit test checks:Small code changes:
ackertoplace_acker(to be consistent withplace_varga) and setacker = place_acker.NamedSignalclass now has a__repr__method that evaluates back to aNamedSignal(similar to theInputOutputSystem__repr__method).Additional changes that are coming: