- Notifications
You must be signed in to change notification settings - Fork441
Time response plot improvements#1018
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
Time response plot improvements#1018
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedJun 29, 2024
control/nlsys.py Outdated
| @@ -1327,8 +1327,8 @@ def input_output_response( | |||
| Parameters | |||
| ---------- | |||
| sys : InputOutputSystem | |||
| Input/output system to simulate. | |||
| sysdata : I/O system or list of I/O systems | |||
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.
It is useful to include a class name here because, in that case, docs generation results in a link to documentation for the class. Unless I missed something, this should beNonlinearIOSystem. This will work even when the type is written as "...or list of...", e.g., compare with thedata parameter ofnyquist_plot:https://python-control.readthedocs.io/en/0.10.0/generated/control.nyquist_plot.html
I will finish reviewing this tomorrow morning. |
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.
Good improvements! This is ready to merge after you consider/apply my comments.
control/ctrlplot.py Outdated
| common_prefix = common_prefix[:last_space] | ||
| prefix_len = len(common_prefix) | ||
| # Look for a common suffice (up to a space) |
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.
| # Look for a commonsuffice (up to a space) | |
| # Look for a commonsuffix (up to a space) |
examples/cds110_lti-systems.ipynb Outdated
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.
Misprints:
- "to define and input/output" -> "to define an input/output"
- "response object, we can be" -> "response object, which can be"
examples/plot_gallery.py Outdated
| # used to compare what things look like between different versions of the | ||
| # library. It is mainly intended for uses by developers to make sure there | ||
| # are no unexpected changes in plot formats, but also has some interest | ||
| # examples of htings you can 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.
| # examples ofhtings you can plot. | |
| # examples ofthings you can plot. |
examples/plot_gallery.py Outdated
| # Create a pdf file for storing the results | ||
| from matplotlib.backends.backend_pdf import PdfPages | ||
| from datetime import date | ||
| git_info = os.popen('git describe').read().strip() |
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.
Should it be considered an error if a Git repository is not found?
I ask becauseos.popen will not raise an exception if the subprocess has errors, sogit_info will be an empty string andplot_gallery.py will continue ifgit describe fails. If you want an exception, I recommendcheck_output:
importsubprocessgit_info=subprocess.check_output(['git','describe'],text=True).strip()
doc/conventions.rst Outdated
| automatically compute the time vector based on the poles and zeros of | ||
| system. If a list of systems is passed, a common time vector will be | ||
| computed and a list of responses will be returned in the form of a | ||
| :class:`TimeResponseList` object. The :func:`force_response` function can |
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.
| :class:`TimeResponseList` object. The:func:`force_response` function can | |
| :class:`TimeResponseList` object. The:func:`forced_response` function can |
doc/conventions.rst Outdated
| :class:`TimeResponseList` object. The :func:`force_response` function can | ||
| also take a list of systems, to which a single common input is applied. | ||
| The :class:`TimeResponseList` object has a `plot()` method that will plot | ||
| each of the reponses in turn, using a sequence of different colors with |
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.
| each of thereponses in turn, using a sequence of different colors with | |
| each of theresponses in turn, using a sequence of different colors with |
doc/conventions.rst Outdated
| For linear time invariant (LTI) systems, the :func:`impulse_response`, | ||
| :func:`initial_response`, and :func:`step_response` functions will | ||
| automatically compute the time vector based on the poles and zeros of | ||
| system. If a list of systems is passed, a common time vector will be |
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.
| system. If a list of systems is passed, a common time vector will be | |
| thesystem. If a list of systems is passed, a common time vector will be |
control/nlsys.py Outdated
| @@ -1317,7 +1317,7 @@ def nlsys( | |||
| def input_output_response( | |||
| sys, T, U=0., X0=0, params=None, ignore_errors=False, | |||
| sysdata, T, U=0., X0=0, params=None, ignore_errors=False, | |||
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.
Changing the parameter namesys tosysdata here and in other functions modified by this pull request is good for clarity to users, but it will break existing calls of the forminput_output_response(sys=ss, ...). As such, the next version probably should be 0.11.0 instead of 0.10.1 (#1020) to emphasize that existing code usingcontrol may need to be updated.
coveralls commentedJul 7, 2024
5ba44b5 intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
This PR makes improvements to the code for plotting time responses, building on some of the improvements made for frequency response plots (#1011):
axkeyword processing to allow arrays or lists + uniform processing for all (time and frequency) plot routines.step_responsetwice and could get different default time intervals).examples/plot_gallery.pythat generates a PDF file with some standard plots to compare changes in basic plotting between PRs and releases.To get a sense of the differences, consider the following code:
Prior to this PR, the output from this code was
Note the fact that both responses are in the same color.
With this PR, the output becomes
Furthermore, you can compute a single time range using the command
which gives a single time range for both systems: