NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

Standardize squeeze processing in frequency response functions#507

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

Jump to bottom

Merged

murrayrm merged 5 commits intopython-control:masterfrommurrayrm:standardize_squeeze

Jan 19, 2021

Merged

Standardize squeeze processing in frequency response functions#507

murrayrm merged 5 commits intopython-control:masterfrommurrayrm:standardize_squeeze

Jan 19, 2021

Conversation

Copy link

Member

murrayrm commentedJan 14, 2021•
edited
Loading

This PR addresses issue#453 by implementing a uniform method for handling thesqueeze keyword in frequency response functions (frequency_response,__call__, etc). This PR matches the changes in PR#511 to improve consistency between time and frequency response, as described in issue#453.

The following behavior is implemented:

squeeze=None (default case): squeeze inputs and outputs, but leave frequencies alone. This is consistent with current behavior.
squeeze=True: numpy squeeze => all singleton axes are removed, consistent with numpy.
squeeze=False: full MIMO, even for SISO. You always get a p x m array for outputs versus inputs.

Additional changes:

A new global defaultcontrol.squeeze_frequency_response that sets the default value for the squeeze keyword (set toNone initially).
Passing a set of frequencies (or complex values) that is not a scalar of a 1D array_like generates aValueError exception.
Updated docstrings
Unit tests to make sure that all of the various cases forsqueeze are correct.
Some minor PEP8 cleanup along the way

murrayrm linked an issue

Jan 14, 2021

that may beclosed by this pull request

proposal to standardize squeezing output from systems#453

Closed

Copy link

coveralls commentedJan 14, 2021•
edited
Loading

Coverage increased (+0.03%) to 87.544% when pullingb338e32 on murrayrm:standardize_squeeze intob47ed08 on python-control:master.

bnavigator reviewed

Jan 14, 2021

View reviewed changes

Copy link

Contributor

bnavigator left a comment

Choose a reason for hiding this comment

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

LGTM

(except for the commit sequence. make sure to rebase/squash before merge)

control/tests/iosys_test.py OutdatedShow resolvedHide resolved

control/tests/lti_test.py OutdatedShow resolvedHide resolved

control/tests/timeresp_test.py OutdatedShow resolvedHide resolved

murrayrm force-pushed thestandardize_squeeze branch from61bba31 toa69a331Compare

January 14, 2021 17:00

unify use of squeeze keyword in frequency response functions

848112d

murrayrm force-pushed thestandardize_squeeze branch froma69a331 to4ec6377Compare

January 14, 2021 17:02

bnavigator requested a review fromsawyerbfuller

January 14, 2021 18:51

Copy link

Contributor

bnavigator commentedJan 14, 2021

https://github.com/python-control/python-control/pull/507/checks?check_run_id=1703414743#step:5:75

control/tests/timeresp_test.py: 18 warnings  /home/runner/work/python-control/python-control/control/statesp.py:1506: UserWarning: Converting MIMO system to SIMO system. Only input 0 is used.    warn("Converting MIMO system to SIMO system. "

Better use pytest.warns to keep the warnings clean.

sawyerbfuller mentioned this pull request

Jan 14, 2021

proposal to standardize squeezing output from systems#453

Closed

murrayrm mentioned this pull request

Jan 17, 2021

standardize time response return values, return_x/squeeze keyword processing#511

Merged

murrayrm changed the title~~Standardize squeeze and return_x in time/frequency response functions~~Standardize squeeze processing in time/frequency response functions

Jan 17, 2021

murrayrm marked this pull request as draft

January 17, 2021 00:49

Copy link

MemberAuthor

murrayrm commentedJan 17, 2021•
edited
Loading

I've pulled out the time response portion of this PR and submitted it separately in#511. Converting this to a draft for now. See discussion in issue#453.

murrayrm added3 commits

January 17, 2021 08:18

unify frequency response processing + unit tests

f367cf6

change control.squeeze to control.squeeze_frequency_response

451d6d2

update squeeze processing for freq response + unit tests, doc updates

90da4fb

murrayrm force-pushed thestandardize_squeeze branch fromb337c9c to90da4fbCompare

January 17, 2021 20:33

murrayrm changed the title~~Standardize squeeze processing in time/frequency response functions~~Standardize squeeze processing in frequency response functions

Jan 17, 2021

murrayrm marked this pull request as ready for review

January 17, 2021 20:39

sawyerbfuller reviewed

Jan 18, 2021

View reviewed changes

control/frdata.py Outdated

		@@ -423,9 +440,14 @@ def __call__(self, s, squeeze=True):
		:class:`FrequencyDomainData` systems are only defined at imaginary
		frequency values.
		"""
		# Make sure that we are operating on a simple list
		if len(np.array(s, ndmin=1).shape) > 1:

Copy link

Contributor

sawyerbfullerJan 18, 2021

Choose a reason for hiding this comment

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

Suggested change

	iflen(np.array(s,ndmin=1).shape)>1:
	iflen(np.atleast1d(s).shape)>1:

sawyerbfuller reviewed

Jan 18, 2021

View reviewed changes

control/frdata.py Outdated

		# Make sure that we are operating on a simple list
		if len(np.array(s, ndmin=1).shape) > 1:
		raise ValueError("input list must be 1D")

		if any(abs(np.array(s, ndmin=1).real) > 0):

Copy link

Contributor

sawyerbfullerJan 18, 2021•
edited
Loading

Choose a reason for hiding this comment

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

Suggested change

	ifany(abs(np.array(s,ndmin=1).real)>0):
	ifany(abs(np.atleast1d(s).real)>0):

sawyerbfuller reviewed

Jan 18, 2021

View reviewed changes

control/statesp.py Outdated

		`m = self.inputs` number of inputs and `p = self.outputs` number of
		outputs.

		In general the system may be multiple input, multiple output

Copy link

Contributor

sawyerbfullerJan 18, 2021

Choose a reason for hiding this comment

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

Can probably remove this paragraph. Was added to precisely specify return array size but now no longer needed

sawyerbfuller reviewed

Jan 18, 2021

View reviewed changes

control/lti.py

		squeeze is False, the array is 3D, indexed by the output, input, and
		frequency. If ``squeeze`` is True then single-dimensional axes are
		removed.
		phase : ndarray

Copy link

Contributor

sawyerbfullerJan 18, 2021

Choose a reason for hiding this comment

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

In ‘notes’ on line 575ish below, this function is actually a wrapper for :meth:’lti.frequency_response’

Copy link

MemberAuthor

murrayrmJan 18, 2021

Choose a reason for hiding this comment

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

Because of the way the documentation is built, theLTI class does not show up, so the documentation appears in the StateSpace and TransferFunction classes. Leaving unchanged.

address@sawyerbfullerreview comments

b338e32

murrayrm merged commit6f674cf intopython-control:master

Jan 19, 2021

murrayrm deleted the standardize_squeeze branch

January 19, 2021 16:53

murrayrm added this to the0.9.0 milestone

Mar 20, 2021

Labels

None yet

4 participants

Movatterモバイル変換

Standardize squeeze processing in frequency response functions#507

Standardize squeeze processing in frequency response functions#507

Uh oh!

Conversation

murrayrm commentedJan 14, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

coveralls commentedJan 14, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

bnavigator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnavigator commentedJan 14, 2021

Uh oh!

murrayrm commentedJan 17, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

sawyerbfullerJan 18, 2021

Choose a reason for hiding this comment

Uh oh!

sawyerbfullerJan 18, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sawyerbfullerJan 18, 2021

Choose a reason for hiding this comment

Uh oh!

sawyerbfullerJan 18, 2021

Choose a reason for hiding this comment

Uh oh!

murrayrmJan 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

murrayrm commentedJan 14, 2021•
edited
Loading

coveralls commentedJan 14, 2021•
edited
Loading

murrayrm commentedJan 17, 2021•
edited
Loading

sawyerbfullerJan 18, 2021•
edited
Loading