Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
murrayrm merged 5 commits into from
Jan 19, 2021
Merged

Standardize squeeze processing in frequency response functions #507

murrayrm merged 5 commits into from
Jan 19, 2021

Conversation

@murrayrm
Copy link
Member

@murrayrm murrayrm commented Jan 14, 2021 •
edited
Loading

This PR addresses issue #453 by implementing a uniform method for handling the squeeze 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 default control.squeeze_frequency_response that sets the default value for the squeeze keyword (set to None initially).
  • Passing a set of frequencies (or complex values) that is not a scalar of a 1D array_like generates a ValueError exception.
  • Updated docstrings
  • Unit tests to make sure that all of the various cases for squeeze are correct.
  • Some minor PEP8 cleanup along the way

@murrayrm murrayrm linked an issue Jan 14, 2021 that may be closed by this pull request
proposal to standardize squeezing output from systems #453
Closed
@coveralls
Copy link

coveralls commented Jan 14, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 87.544% when pulling b338e32 on murrayrm:standardize_squeeze into b47ed08 on python-control:master.

bnavigator
bnavigator reviewed Jan 14, 2021
View reviewed changes
Copy link
Contributor

@bnavigator 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)

@murrayrm murrayrm force-pushed the standardize_squeeze branch from 61bba31 to a69a331 Compare January 14, 2021 17:00
@murrayrm murrayrm force-pushed the standardize_squeeze branch from a69a331 to 4ec6377 Compare January 14, 2021 17:02
@bnavigator
Copy link
Contributor

bnavigator commented Jan 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 sawyerbfuller mentioned this pull request Jan 14, 2021
Closed
@murrayrm murrayrm mentioned this pull request Jan 17, 2021
Merged
@murrayrm 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 murrayrm marked this pull request as draft January 17, 2021 00:49
@murrayrm
Copy link
Member Author

murrayrm commented Jan 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 murrayrm force-pushed the standardize_squeeze branch from b337c9c to 90da4fb Compare January 17, 2021 20:33
@murrayrm murrayrm changed the title Standardize squeeze processing in time/frequency response functions Standardize squeeze processing in frequency response functions Jan 17, 2021
@murrayrm murrayrm marked this pull request as ready for review January 17, 2021 20:39
sawyerbfuller
sawyerbfuller reviewed Jan 18, 2021
View reviewed changes
control/frdata.py Outdated
frequency values.
"""
# Make sure that we are operating on a simple list
if len(np.array(s, ndmin=1).shape) > 1:
Copy link
Contributor

@sawyerbfuller sawyerbfuller Jan 18, 2021

Choose a reason for hiding this comment

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

Suggested change
if len(np.array(s, ndmin=1).shape) > 1:
if len(np.atleast1d(s).shape) > 1:

sawyerbfuller
sawyerbfuller reviewed Jan 18, 2021
View reviewed changes
control/frdata.py Outdated
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

@sawyerbfuller sawyerbfuller Jan 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
if any(abs(np.array(s, ndmin=1).real) > 0):
if any(abs(np.atleast1d(s).real) > 0):

sawyerbfuller
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

@sawyerbfuller sawyerbfuller Jan 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
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

@sawyerbfuller sawyerbfuller Jan 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
Member Author

@murrayrm murrayrm Jan 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, the LTI class does not show up, so the documentation appears in the StateSpace and TransferFunction classes. Leaving unchanged.

@murrayrm murrayrm merged commit 6f674cf into python-control:master Jan 19, 2021
@murrayrm murrayrm deleted the standardize_squeeze branch January 19, 2021 16:53
@murrayrm murrayrm added this to the 0.9.0 milestone Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnavigator bnavigator bnavigator left review comments

@sawyerbfuller sawyerbfuller sawyerbfuller left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

proposal to standardize squeezing output from systems

4 participants

@murrayrm @coveralls @bnavigator @sawyerbfuller