GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Standardize squeeze processing in frequency response functions by murrayrm · Pull Request #507 · python-control/python-control

Conversation

@murrayrm

@murrayrm murrayrm commented Jan 14, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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

coveralls commented Jan 14, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnavigator
bnavigator reviewed Jan 14, 2021
View reviewed changes

@bnavigator bnavigator left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/tests/iosys_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/iosys_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/lti_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/timeresp_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/timeresp_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@murrayrm
murrayrm force-pushed the standardize_squeeze branch from 61bba31 to a69a331 Compare January 14, 2021 17:00
@murrayrm
unify use of squeeze keyword in frequency response functions
848112d
@murrayrm
murrayrm force-pushed the standardize_squeeze branch from a69a331 to 4ec6377 Compare January 14, 2021 17:02
@bnavigator
bnavigator requested a review from sawyerbfuller January 14, 2021 18:51
@bnavigator

bnavigator commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller sawyerbfuller mentioned this pull request Jan 14, 2021
proposal to standardize squeezing output from systems #453
Closed
@murrayrm murrayrm mentioned this pull request Jan 17, 2021
standardize time response return values, return_x/squeeze keyword processing #511
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

murrayrm commented Jan 17, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
murrayrm added 3 commits January 17, 2021 08:18
@murrayrm
unify frequency response processing + unit tests
f367cf6
@murrayrm
change control.squeeze to control.squeeze_frequency_response
451d6d2
@murrayrm
update squeeze processing for freq response + unit tests, doc updates
90da4fb
@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
Comment thread 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:

@sawyerbfuller sawyerbfuller Jan 18, 2021

Copy link
Copy Markdown
Contributor

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
if len(np.array(s, ndmin=1).shape) > 1:
if len(np.atleast1d(s).shape) > 1:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Jan 18, 2021
View reviewed changes
Comment thread 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):

@sawyerbfuller sawyerbfuller Jan 18, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Jan 18, 2021
View reviewed changes
Comment thread 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

@sawyerbfuller sawyerbfuller Jan 18, 2021

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Jan 18, 2021
View reviewed changes
Comment thread 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

@sawyerbfuller sawyerbfuller Jan 18, 2021

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Jan 18, 2021

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@murrayrm
address @sawyerbfuller review comments
b338e32
@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

Uh oh!

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

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

Uh oh!

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

4 participants

@murrayrm @coveralls @bnavigator @sawyerbfuller
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.