GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Time response plot improvements by murrayrm · Pull Request #1018 · python-control/python-control

Conversation

murrayrm commented Jun 29, 2024

Copy link

Copy Markdown

Member

This PR makes improvements to the code for plotting time responses, building on some of the improvements made for frequency response plots (#1011):

Fix up the ax keyword processing to allow arrays or lists + uniform processing for all (time and frequency) plot routines.
Slight refactoring of code to put common functions in ctrlplot.py (additional changes coming in future PR to further consolidate common code)
Allow time responses for multiple systems with common time vector and inputs. This enables commands like
```
ct.step_response([sys1, sys2]).plot
```
to find a single time interval (before you had to call step_response twice and could get different default time intervals).
Fix sequential plotting so that different colors are used and plot title is updated (like Bode and Nyquist).
Allow label keyword in various time response commands to override default label generation.
Allow legends to be turned on and off using show_legend keyword.
Added a file examples/plot_gallery.py that 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:

sys_mimo1 = ct.tf2ss(
    [[[1], [0.1]], [[0.2], [1]]],
    [[[1, 0.6, 1], [1, 1, 1]], [[1, 0.4, 1], [1, 2, 1]]], name="sys_mimo1")
sys_mimo2 = ct.tf2ss(
    [[[1], [0.1]], [[0.2], [1]]],
    [[[1, 0.2, 1], [1, 24, 22, 5]], [[1, 4, 16, 21], [1, 0.1]]],
    name="sys_mimo2")
ct.step_response(sys_mimo1).plot()
ct.step_response(sys_mimo2).plot()

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

ct.step_response([sys_mimo1, sys_mimo2])

which gives a single time range for both systems:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ilayn and KybernetikJo reacted with thumbs up emoji

All reactions

👍 2 reactions

murrayrm added 11 commits

June 29, 2024 12:41


          initial pass at defining lists of systems for time responses

95326b0


          use _process_ax_keyword in time_response_plot


          refactoring to put common functions in ctrlplot.py

44123c9


          Add list of systems functionality to time response functions

8b416eb


          allow combining I/O time responses with different numbers of states

6be8f48


          add plot_gallery to compare plotting results between builds

8b269bf


          improved unit tests for rcParams + small fix to suptitle rcParams

cc6aeb6


          allow label keyword to override generated labels

ed8a1c1


          cleanup after plotting in test functions

92d36cc


          add CDS 110 example notebook demonstrating features

8f0c227


          add show_legend processing

d459569

coveralls commented Jun 29, 2024

Copy link

Copy Markdown

coverage: 94.524% (+0.006%) from 94.518%
when pulling d459569 on murrayrm:timeresp_improvements-01Jun2024
into feeb56a on python-control:main.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

slivingston self-requested a review

June 29, 2024 22:54

slivingston self-assigned this

Jun 29, 2024

murrayrm added this to the 0.10.1 milestone

Jun 30, 2024

murrayrm mentioned this pull request

Jun 30, 2024

Version 0.10.1 release plans #1020

Closed

11 tasks done

slivingston reviewed

Jul 2, 2024

View reviewed changes

Comment thread

control/nlsys.py Outdated

		----------
		sys : InputOutputSystem
		Input/output system to simulate.
		sysdata : I/O system or list of I/O systems

slivingston Jul 2, 2024

Copy link

Copy Markdown

Member

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 be NonlinearIOSystem. This will work even when the type is written as "...or list of...", e.g., compare with the data parameter of nyquist_plot: https://python-control.readthedocs.io/en/0.10.0/generated/control.nyquist_plot.html

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

slivingston commented Jul 3, 2024

Copy link

Copy Markdown

Member

I will finish reviewing this tomorrow morning.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

slivingston reviewed

Jul 3, 2024

View reviewed changes

slivingston left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/ctrlplot.py Outdated

		common_prefix = common_prefix[:last_space]
		prefix_len = len(common_prefix)

		# Look for a common suffice (up to a space)

slivingston Jul 2, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	# Look for a common suffice (up to a space)
	# Look for a common suffix (up to a space)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

examples/cds110_lti-systems.ipynb

slivingston Jul 3, 2024

Copy link

Copy Markdown

Member

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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.

slivingston Jul 3, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	# examples of htings you can plot.
	# examples of things you can plot.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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()

slivingston Jul 3, 2024

Copy link

Copy Markdown

Member

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 because os.popen will not raise an exception if the subprocess has errors, so git_info will be an empty string and plot_gallery.py will continue if git describe fails. If you want an exception, I recommend check_output:

import subprocess
git_info = subprocess.check_output(['git', 'describe'], text=True).strip()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

slivingston Jul 3, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	:class:`TimeResponseList` object. The :func:`force_response` function can
	:class:`TimeResponseList` object. The :func:`forced_response` function can

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

slivingston Jul 3, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	each of the reponses in turn, using a sequence of different colors with
	each of the responses in turn, using a sequence of different colors with

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

slivingston Jul 3, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	system. If a list of systems is passed, a common time vector will be
	the system. If a list of systems is passed, a common time vector will be

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/nlsys.py Outdated


		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,

slivingston Jul 3, 2024

Copy link

Copy Markdown

Member

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 name sys to sysdata here and in other functions modified by this pull request is good for clarity to users, but it will break existing calls of the form input_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 using control may need to be updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          address @slivingston review comments

10f009b

coveralls commented Jul 7, 2024

Copy link

Copy Markdown

coverage: 94.554% (+0.04%) from 94.518%
when pulling 10f009b on murrayrm:timeresp_improvements-01Jun2024
into feeb56a on python-control:main.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm merged commit 5ba44b5 into python-control:main

Jul 7, 2024

murrayrm deleted the timeresp_improvements-01Jun2024 branch

July 7, 2024 02:33

Reviewers

slivingston

slivingston left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

slivingston

Labels

None yet

Projects

None yet

Milestone

0.10.1

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.