Standardize squeeze processing in frequency response functions #507

murrayrm · 2021-01-14T01:46:23Z

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

coveralls · 2021-01-14T01:50:26Z

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

bnavigator

LGTM

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

control/tests/iosys_test.py

control/tests/lti_test.py

control/tests/timeresp_test.py

bnavigator · 2021-01-14T19:09:31Z

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.

murrayrm · 2021-01-17T00:51:26Z

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.

sawyerbfuller · 2021-01-18T13:11:14Z

control/frdata.py

@@ -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:


Suggested change

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

if len(np.atleast1d(s).shape) > 1:

sawyerbfuller · 2021-01-18T13:39:25Z

control/frdata.py

+        # 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):


Suggested change

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

if any(abs(np.atleast1d(s).real) > 0):

sawyerbfuller · 2021-01-18T13:41:59Z

control/statesp.py

-        `m = self.inputs` number of inputs and `p = self.outputs` number of
-        outputs.
+
+        In general the system may be multiple input, multiple output


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

sawyerbfuller · 2021-01-18T14:13:07Z

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


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

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 linked an issue Jan 14, 2021 that may be closed by this pull request

proposal to standardize squeezing output from systems #453

Closed

bnavigator reviewed Jan 14, 2021

View reviewed changes

murrayrm force-pushed the standardize_squeeze branch from 61bba31 to a69a331 Compare January 14, 2021 17:00

unify use of squeeze keyword in frequency response functions

848112d

murrayrm force-pushed the standardize_squeeze branch from a69a331 to 4ec6377 Compare January 14, 2021 17:02

bnavigator requested a review from sawyerbfuller January 14, 2021 18:51

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

murrayrm added 3 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 the standardize_squeeze branch from b337c9c to 90da4fb Compare 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

address @sawyerbfuller review comments

b338e32

murrayrm merged commit 6f674cf into python-control:master Jan 19, 2021

murrayrm deleted the standardize_squeeze branch January 19, 2021 16:53

murrayrm added this to the 0.9.0 milestone Mar 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize squeeze processing in frequency response functions #507

Standardize squeeze processing in frequency response functions #507

murrayrm commented Jan 14, 2021 •

edited

Loading

coveralls commented Jan 14, 2021 •

edited

Loading

bnavigator left a comment

bnavigator commented Jan 14, 2021

murrayrm commented Jan 17, 2021 •

edited

Loading

sawyerbfuller Jan 18, 2021

sawyerbfuller Jan 18, 2021 •

edited

Loading

sawyerbfuller Jan 18, 2021

sawyerbfuller Jan 18, 2021

murrayrm Jan 18, 2021

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

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

Standardize squeeze processing in frequency response functions #507

Standardize squeeze processing in frequency response functions #507

Conversation

murrayrm commented Jan 14, 2021 • edited Loading

coveralls commented Jan 14, 2021 • edited Loading

bnavigator left a comment

Choose a reason for hiding this comment

bnavigator commented Jan 14, 2021

murrayrm commented Jan 17, 2021 • edited Loading

sawyerbfuller Jan 18, 2021

Choose a reason for hiding this comment

sawyerbfuller Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

sawyerbfuller Jan 18, 2021

Choose a reason for hiding this comment

sawyerbfuller Jan 18, 2021

Choose a reason for hiding this comment

murrayrm Jan 18, 2021

Choose a reason for hiding this comment

murrayrm commented Jan 14, 2021 •

edited

Loading

coveralls commented Jan 14, 2021 •

edited

Loading

murrayrm commented Jan 17, 2021 •

edited

Loading

sawyerbfuller Jan 18, 2021 •

edited

Loading