From 8b6bfd199de4a0f820be577496b7392560ececb7 Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Sun, 20 Mar 2022 06:20:19 -0700 Subject: [PATCH 1/8] check for unused keywords --- control/frdata.py | 6 +++++- control/freqplot.py | 6 ++---- control/iosys.py | 9 +++++---- control/optimal.py | 2 +- control/pzmap.py | 6 +++++- control/rlocus.py | 4 ++++ control/tests/interconnect_test.py | 10 +++++----- control/tests/trdata_test.py | 2 +- control/timeresp.py | 6 +++--- 9 files changed, 31 insertions(+), 20 deletions(-) diff --git a/control/frdata.py b/control/frdata.py index c43a241e4..19e865821 100644 --- a/control/frdata.py +++ b/control/frdata.py @@ -150,7 +150,11 @@ def __init__(self, *args, **kwargs): """ # TODO: discrete-time FRD systems? - smooth = kwargs.get('smooth', False) + smooth = kwargs.pop('smooth', False) + + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) if len(args) == 2: if not isinstance(args[0], FRD) and isinstance(args[0], LTI): diff --git a/control/freqplot.py b/control/freqplot.py index a8324e06e..7a1243c6c 100644 --- a/control/freqplot.py +++ b/control/freqplot.py @@ -221,12 +221,10 @@ def bode_plot(syslist, omega=None, # Get the current figure if 'sisotool' in kwargs: - fig = kwargs['fig'] + fig = kwargs.pop('fig') ax_mag = fig.axes[0] ax_phase = fig.axes[2] - sisotool = kwargs['sisotool'] - del kwargs['fig'] - del kwargs['sisotool'] + sisotool = kwargs.pop('sisotool') else: fig = plt.gcf() ax_mag = None diff --git a/control/iosys.py b/control/iosys.py index d5a7b755b..5f606ee93 100644 --- a/control/iosys.py +++ b/control/iosys.py @@ -148,7 +148,7 @@ def __init__(self, inputs=None, outputs=None, states=None, params={}, # timebase self.dt = kwargs.pop('dt', config.defaults['control.default_dt']) - # Make sure there were no extraneous keyworks + # Make sure there were no extraneous keywords if kwargs: raise TypeError("unrecognized keywords: ", str(kwargs)) @@ -790,9 +790,9 @@ def __init__(self, updfcn, outfcn=None, inputs=None, outputs=None, params=params, dt=dt, name=name ) - # Make sure all input arguments got parsed + # Make sure there were no extraneous keywords if kwargs: - raise TypeError("unknown parameters %s" % kwargs) + raise TypeError("unrecognized keywords: ", str(kwargs)) # Check to make sure arguments are consistent if updfcn is None: @@ -2134,7 +2134,8 @@ def _parse_signal_parameter(value, name, kwargs, end=False): value = kwargs.pop(name) if end and kwargs: - raise TypeError("unknown parameters %s" % kwargs) + raise TypeError("unrecognized keywords: ", str(kwargs)) + return value diff --git a/control/optimal.py b/control/optimal.py index cb9f57ded..aea9b02b8 100644 --- a/control/optimal.py +++ b/control/optimal.py @@ -154,7 +154,7 @@ def __init__( self.minimize_kwargs.update(kwargs.pop( 'minimize_kwargs', config.defaults['optimal.minimize_kwargs'])) - # Make sure all input arguments got parsed + # Make sure there were no extraneous keywords if kwargs: raise TypeError("unrecognized keyword(s): ", str(kwargs)) diff --git a/control/pzmap.py b/control/pzmap.py index ae8db1241..7d3836d7f 100644 --- a/control/pzmap.py +++ b/control/pzmap.py @@ -91,7 +91,11 @@ def pzmap(sys, plot=None, grid=None, title='Pole Zero Map', **kwargs): import warnings warnings.warn("'Plot' keyword is deprecated in pzmap; use 'plot'", FutureWarning) - plot = kwargs['Plot'] + plot = kwargs.pop('Plot') + + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) # Get parameter values plot = config._get_param('pzmap', 'plot', plot, True) diff --git a/control/rlocus.py b/control/rlocus.py index 23122fe72..5cf7983a3 100644 --- a/control/rlocus.py +++ b/control/rlocus.py @@ -168,6 +168,10 @@ def root_locus(sys, kvect=None, xlim=None, ylim=None, # Check for sisotool mode sisotool = False if 'sisotool' not in kwargs else True + # Make sure there were no extraneous keywords + if not sisotool and kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) + # Create the Plot if plot: if sisotool: diff --git a/control/tests/interconnect_test.py b/control/tests/interconnect_test.py index dd31241e7..3b99adc6e 100644 --- a/control/tests/interconnect_test.py +++ b/control/tests/interconnect_test.py @@ -188,19 +188,19 @@ def test_interconnect_exceptions(): # Unrecognized arguments # LinearIOSystem - with pytest.raises(TypeError, match="unknown parameter"): + with pytest.raises(TypeError, match="unrecognized keyword"): P = ct.LinearIOSystem(ct.rss(2, 1, 1), output_name='y') # Interconnect - with pytest.raises(TypeError, match="unknown parameter"): + with pytest.raises(TypeError, match="unrecognized keyword"): T = ct.interconnect((P, C, sumblk), input_name='r', output='y') # Interconnected system - with pytest.raises(TypeError, match="unknown parameter"): + with pytest.raises(TypeError, match="unrecognized keyword"): T = ct.InterconnectedSystem((P, C, sumblk), input_name='r', output='y') # NonlinearIOSytem - with pytest.raises(TypeError, match="unknown parameter"): + with pytest.raises(TypeError, match="unrecognized keyword"): nlios = ct.NonlinearIOSystem( None, lambda t, x, u, params: u*u, input_count=1, output_count=1) @@ -208,7 +208,7 @@ def test_interconnect_exceptions(): with pytest.raises(TypeError, match="input specification is required"): sumblk = ct.summing_junction() - with pytest.raises(TypeError, match="unknown parameter"): + with pytest.raises(TypeError, match="unrecognized keyword"): sumblk = ct.summing_junction(input_count=2, output_count=2) diff --git a/control/tests/trdata_test.py b/control/tests/trdata_test.py index fcd8676e9..734d35599 100644 --- a/control/tests/trdata_test.py +++ b/control/tests/trdata_test.py @@ -208,7 +208,7 @@ def test_response_copy(): assert response.input_labels == ['u'] # Unknown keyword - with pytest.raises(ValueError, match="Unknown parameter(s)*"): + with pytest.raises(TypeError, match="unrecognized keywords"): response_bad_kw = response_mimo(input=0) diff --git a/control/timeresp.py b/control/timeresp.py index e2ce822f6..bf826b539 100644 --- a/control/timeresp.py +++ b/control/timeresp.py @@ -480,9 +480,9 @@ def __call__(self, **kwargs): response.state_labels = _process_labels( state_labels, "state", response.nstates) - # Make sure no unknown keywords were passed - if len(kwargs) != 0: - raise ValueError("Unknown parameter(s) %s" % kwargs) + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) return response From 3656968a55ea50f9c103c51f14b9aa789fde9938 Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Sun, 20 Mar 2022 06:24:48 -0700 Subject: [PATCH 2/8] remove_useless -> remove_useless_states in iosys --- control/iosys.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/control/iosys.py b/control/iosys.py index 5f606ee93..c2fe62e40 100644 --- a/control/iosys.py +++ b/control/iosys.py @@ -560,7 +560,7 @@ def linearize(self, x0, u0, t=0, params={}, eps=1e-6, # Create the state space system linsys = LinearIOSystem( - StateSpace(A, B, C, D, self.dt, remove_useless=False), + StateSpace(A, B, C, D, self.dt, remove_useless_states=False), name=name, **kwargs) # Set the names the system, inputs, outputs, and states @@ -660,7 +660,7 @@ def __init__(self, linsys, inputs=None, outputs=None, states=None, states=linsys.nstates, params={}, dt=linsys.dt, name=name) # Initalize additional state space variables - StateSpace.__init__(self, linsys, remove_useless=False) + StateSpace.__init__(self, linsys, remove_useless_states=False) # Process input, output, state lists, if given # Make sure they match the size of the linear system @@ -1551,7 +1551,7 @@ def __init__(self, io_sys, ss_sys=None): io_sys.nstates != ss_sys.nstates: raise ValueError("System dimensions for first and second " "arguments must match.") - StateSpace.__init__(self, ss_sys, remove_useless=False) + StateSpace.__init__(self, ss_sys, remove_useless_states=False) else: raise TypeError("Second argument must be a state space system.") From ceddc6f0e60262958a60b31cb709e39860480076 Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Sun, 20 Mar 2022 06:33:35 -0700 Subject: [PATCH 3/8] add test for extraneous keywords in iosys --- control/iosys.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/control/iosys.py b/control/iosys.py index c2fe62e40..196b8968b 100644 --- a/control/iosys.py +++ b/control/iosys.py @@ -1656,14 +1656,14 @@ def input_output_response( raise ValueError("ivp_method specified more than once") solve_ivp_kwargs['method'] = kwargs.pop('solve_ivp_method') + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) + # Set the default method to 'RK45' if solve_ivp_kwargs.get('method', None) is None: solve_ivp_kwargs['method'] = 'RK45' - # Make sure all input arguments got parsed - if kwargs: - raise TypeError("unknown parameters %s" % kwargs) - # Sanity checking on the input if not isinstance(sys, InputOutputSystem): raise TypeError("System of type ", type(sys), " not valid") From 97a0a14b9341c0b2c5b2f2957d83aca3852b6794 Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Sun, 20 Mar 2022 06:48:03 -0700 Subject: [PATCH 4/8] add test for extraneous keywords in xferfcn --- control/xferfcn.py | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/control/xferfcn.py b/control/xferfcn.py index df1b6d404..76d704afc 100644 --- a/control/xferfcn.py +++ b/control/xferfcn.py @@ -265,6 +265,10 @@ def __init__(self, *args, **kwargs): dt = config.defaults['control.default_dt'] self.dt = dt + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) + # # Class attributes # @@ -1297,7 +1301,7 @@ def _add_siso(num1, den1, num2, den2): return num, den -def _convert_to_transfer_function(sys, **kw): +def _convert_to_transfer_function(sys, inputs=1, outputs=1): """Convert a system to transfer function form (if needed). If sys is already a transfer function, then it is returned. If sys is a @@ -1324,13 +1328,9 @@ def _convert_to_transfer_function(sys, **kw): from .statesp import StateSpace if isinstance(sys, TransferFunction): - if len(kw): - raise TypeError("If sys is a TransferFunction, " + - "_convertToTransferFunction cannot take keywords.") - return sys - elif isinstance(sys, StateSpace): + elif isinstance(sys, StateSpace): if 0 == sys.nstates: # Slycot doesn't like static SS->TF conversion, so handle # it first. Can't join this with the no-Slycot branch, @@ -1341,14 +1341,9 @@ def _convert_to_transfer_function(sys, **kw): for i in range(sys.noutputs)] else: try: - from slycot import tb04ad - if len(kw): - raise TypeError( - "If sys is a StateSpace, " + - "_convertToTransferFunction cannot take keywords.") - # Use Slycot to make the transformation # Make sure to convert system matrices to numpy arrays + from slycot import tb04ad tfout = tb04ad( sys.nstates, sys.ninputs, sys.noutputs, array(sys.A), array(sys.B), array(sys.C), array(sys.D), tol1=0.0) @@ -1381,15 +1376,6 @@ def _convert_to_transfer_function(sys, **kw): return TransferFunction(num, den, sys.dt) elif isinstance(sys, (int, float, complex, np.number)): - if "inputs" in kw: - inputs = kw["inputs"] - else: - inputs = 1 - if "outputs" in kw: - outputs = kw["outputs"] - else: - outputs = 1 - num = [[[sys] for j in range(inputs)] for i in range(outputs)] den = [[[1] for j in range(inputs)] for i in range(outputs)] @@ -1498,6 +1484,10 @@ def tf(*args, **kwargs): if len(args) == 2 or len(args) == 3: return TransferFunction(*args, **kwargs) elif len(args) == 1: + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) + # Look for special cases defining differential/delay operator if args[0] == 's': return TransferFunction.s @@ -1525,8 +1515,8 @@ def ss2tf(*args, **kwargs): The function accepts either 1 or 4 parameters: ``ss2tf(sys)`` - Convert a linear system from state space into transfer function form. Always creates a - new system. + Convert a linear system from state space into transfer function + form. Always creates a new system. ``ss2tf(A, B, C, D)`` Create a transfer function system from the matrices of its state and From 57e3751ffabc5385898eaaaa6576431906ccbd3a Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Sun, 20 Mar 2022 07:03:17 -0700 Subject: [PATCH 5/8] add test for extraneous keywords in statesp --- control/statesp.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/control/statesp.py b/control/statesp.py index 36682532c..960fdc157 100644 --- a/control/statesp.py +++ b/control/statesp.py @@ -1776,7 +1776,12 @@ def _ss(*args, keywords=None, **kwargs): """Internal function to create StateSpace system""" if len(args) == 4 or len(args) == 5: return StateSpace(*args, keywords=keywords, **kwargs) + elif len(args) == 1: + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) + from .xferfcn import TransferFunction sys = args[0] if isinstance(sys, StateSpace): From a86d33fa0b1f817b776d5d3bb357e919e27898cd Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Sun, 20 Mar 2022 22:36:30 -0700 Subject: [PATCH 6/8] add kwargs_test + checks for unrecognized keywords --- control/iosys.py | 10 ++- control/statefbk.py | 18 ++-- control/statesp.py | 4 + control/tests/frd_test.py | 6 ++ control/tests/iosys_test.py | 13 ++- control/tests/kwargs_test.py | 168 +++++++++++++++++++++++++++++++++++ control/xferfcn.py | 6 +- 7 files changed, 208 insertions(+), 17 deletions(-) create mode 100644 control/tests/kwargs_test.py diff --git a/control/iosys.py b/control/iosys.py index 196b8968b..eeb483227 100644 --- a/control/iosys.py +++ b/control/iosys.py @@ -2238,7 +2238,15 @@ def ss(*args, **kwargs): >>> sys2 = ss(sys_tf) """ - sys = _ss(*args, keywords=kwargs) + # Extract the keyword arguments needed for StateSpace (via _ss) + ss_kwlist = ('dt', 'remove_useless_states') + ss_kwargs = {} + for kw in ss_kwlist: + if kw in kwargs: + ss_kwargs[kw] = kwargs.pop(kw) + + # Create the statespace system and then convert to I/O system + sys = _ss(*args, keywords=ss_kwargs) return LinearIOSystem(sys, **kwargs) diff --git a/control/statefbk.py b/control/statefbk.py index 6598eeeb8..099baa225 100644 --- a/control/statefbk.py +++ b/control/statefbk.py @@ -261,7 +261,7 @@ def place_varga(A, B, p, dtime=False, alpha=None): # contributed by Sawyer B. Fuller -def lqe(*args, **keywords): +def lqe(*args, method=None): """lqe(A, G, C, QN, RN, [, NN]) Linear quadratic estimator design (Kalman filter) for continuous-time @@ -356,18 +356,15 @@ def lqe(*args, **keywords): # Process the arguments and figure out what inputs we received # - # Get the method to use (if specified as a keyword) - method = keywords.get('method', None) + # If we were passed a discrete time system as the first arg, use dlqe() + if isinstance(args[0], LTI) and isdtime(args[0], strict=True): + # Call dlqe + return dlqe(*args, method=method) # Get the system description if (len(args) < 3): raise ControlArgument("not enough input arguments") - # If we were passed a discrete time system as the first arg, use dlqe() - if isinstance(args[0], LTI) and isdtime(args[0], strict=True): - # Call dlqe - return dlqe(*args, **keywords) - # If we were passed a state space system, use that to get system matrices if isinstance(args[0], StateSpace): A = np.array(args[0].A, ndmin=2, dtype=float) @@ -409,7 +406,7 @@ def lqe(*args, **keywords): # contributed by Sawyer B. Fuller -def dlqe(*args, **keywords): +def dlqe(*args, method=None): """dlqe(A, G, C, QN, RN, [, N]) Linear quadratic estimator design (Kalman filter) for discrete-time @@ -480,9 +477,6 @@ def dlqe(*args, **keywords): # Process the arguments and figure out what inputs we received # - # Get the method to use (if specified as a keyword) - method = keywords.get('method', None) - # Get the system description if (len(args) < 3): raise ControlArgument("not enough input arguments") diff --git a/control/statesp.py b/control/statesp.py index 960fdc157..435ff702f 100644 --- a/control/statesp.py +++ b/control/statesp.py @@ -340,6 +340,10 @@ def __init__(self, *args, keywords=None, **kwargs): self.dt = dt self.nstates = A.shape[1] + # Make sure there were no extraneous keywords + if keywords: + raise TypeError("unrecognized keywords: ", str(keywords)) + if 0 == self.nstates: # static gain # matrix's default "empty" shape is 1x0 diff --git a/control/tests/frd_test.py b/control/tests/frd_test.py index c63a4c02b..af7d18bc1 100644 --- a/control/tests/frd_test.py +++ b/control/tests/frd_test.py @@ -472,3 +472,9 @@ def test_repr_str(self): 10.000 0.2 +4j 100.000 0.1 +6j""" assert str(sysm) == refm + + def test_unrecognized_keyword(self): + h = TransferFunction([1], [1, 2, 2]) + omega = np.logspace(-1, 2, 10) + with pytest.raises(TypeError, match="unrecognized keyword"): + frd = FRD(h, omega, unknown=None) diff --git a/control/tests/iosys_test.py b/control/tests/iosys_test.py index e76a6be55..93ce5df65 100644 --- a/control/tests/iosys_test.py +++ b/control/tests/iosys_test.py @@ -1580,7 +1580,8 @@ def test_interconnect_unused_input(): outputs=['u'], name='k') - with pytest.warns(UserWarning, match=r"Unused input\(s\) in InterconnectedSystem"): + with pytest.warns( + UserWarning, match=r"Unused input\(s\) in InterconnectedSystem"): h = ct.interconnect([g,s,k], inputs=['r'], outputs=['y']) @@ -1611,13 +1612,19 @@ def test_interconnect_unused_input(): # warn if explicity ignored input in fact used - with pytest.warns(UserWarning, match=r"Input\(s\) specified as ignored is \(are\) used:") as record: + with pytest.warns( + UserWarning, + match=r"Input\(s\) specified as ignored is \(are\) used:") \ + as record: h = ct.interconnect([g,s,k], inputs=['r'], outputs=['y'], ignore_inputs=['u','n']) - with pytest.warns(UserWarning, match=r"Input\(s\) specified as ignored is \(are\) used:") as record: + with pytest.warns( + UserWarning, + match=r"Input\(s\) specified as ignored is \(are\) used:") \ + as record: h = ct.interconnect([g,s,k], inputs=['r'], outputs=['y'], diff --git a/control/tests/kwargs_test.py b/control/tests/kwargs_test.py new file mode 100644 index 000000000..089d910c8 --- /dev/null +++ b/control/tests/kwargs_test.py @@ -0,0 +1,168 @@ +# kwargs_test.py - test for uncrecognized keywords +# RMM, 20 Mar 2022 +# +# Allowing unrecognized keywords to be passed to a function without +# generating and error message can generate annoying bugs, since you +# sometimes think you are telling the function to do something and actually +# you have a misspelling or other error and your input is being ignored. +# +# This unit test looks through all functions in the package for any that +# allow kwargs as part of the function signature and makes sure that there +# is a unit test that checks for unrecognized keywords. + +import inspect +import pytest +import warnings + +import control +import control.flatsys + +# List of all of the test modules where kwarg unit tests are defined +import control.tests.flatsys_test as flatsys_test +import control.tests.frd_test as frd_test +import control.tests.interconnect_test as interconnect_test +import control.tests.statefbk_test as statefbk_test +import control.tests.trdata_test as trdata_test + + +@pytest.mark.parametrize("module, prefix", [ + (control, ""), (control.flatsys, "flatsys.") +]) +def test_kwarg_search(module, prefix): + # Look through every object in the package + for name, obj in inspect.getmembers(module): + # Skip anything that is outside of this module + if inspect.getmodule(obj) is not None and \ + not inspect.getmodule(obj).__name__.startswith('control'): + # Skip anything that isn't part of the control package + continue + + # Look for functions with keyword arguments + if inspect.isfunction(obj): + # Get the signature for the function + sig = inspect.signature(obj) + + # See if there is a variable keyword argument + for argname, par in sig.parameters.items(): + if par.kind == inspect.Parameter.VAR_KEYWORD: + # Make sure there is a unit test defined + assert prefix + name in kwarg_unittest + + # Make sure there is a unit test + if not hasattr(kwarg_unittest[prefix + name], '__call__'): + warnings.warn("No unit test defined for '%s'" + % prefix + name) + + # Look for classes and then check member functions + if inspect.isclass(obj): + test_kwarg_search(obj, prefix + obj.__name__ + '.') + + +# Create a SISO system for use in parameterized tests +sys = control.ss([[-1, 1], [0, -1]], [[0], [1]], [[1, 0]], 0, dt=None) + + +# Parameterized tests for looking for unrecognized keyword errors +@pytest.mark.parametrize("function, args, kwargs", [ + [control.dlqr, (sys, [[1, 0], [0, 1]], [[1]]), {}], + [control.drss, (2, 1, 1), {}], + [control.input_output_response, (sys, [0, 1, 2], [1, 1, 1]), {}], + [control.lqr, (sys, [[1, 0], [0, 1]], [[1]]), {}], + [control.pzmap, (sys,), {}], + [control.rlocus, (control.tf([1], [1, 1]), ), {}], + [control.root_locus, (control.tf([1], [1, 1]), ), {}], + [control.rss, (2, 1, 1), {}], + [control.ss, (0, 0, 0, 0), {'dt': 1}], + [control.ss2io, (sys,), {}], + [control.summing_junction, (2,), {}], + [control.tf, ([1], [1, 1]), {}], + [control.tf2io, (control.tf([1], [1, 1]),), {}], + [control.InputOutputSystem, (1, 1, 1), {}], + [control.StateSpace, ([[-1, 0], [0, -1]], [[1], [1]], [[1, 1]], 0), {}], + [control.TransferFunction, ([1], [1, 1]), {}], +]) +def test_unrecognized_kwargs(function, args, kwargs): + # Call the function normally and make sure it works + function(*args, **kwargs) + + # Now add an unrecognized keyword and make sure there is an error + with pytest.raises(TypeError, match="unrecognized keyword"): + function(*args, **kwargs, unknown=None) + + +# Parameterized tests for looking for keyword errors handled by matplotlib +@pytest.mark.parametrize("function, args, kwargs", [ + [control.bode, (sys, ), {}], + [control.bode_plot, (sys, ), {}], + [control.gangof4, (sys, sys), {}], + [control.gangof4_plot, (sys, sys), {}], + [control.nyquist, (sys, ), {}], + [control.nyquist_plot, (sys, ), {}], +]) +def test_matplotlib_kwargs(function, args, kwargs): + # Call the function normally and make sure it works + function(*args, **kwargs) + + # Now add an unrecognized keyword and make sure there is an error + with pytest.raises(AttributeError, match="has no property"): + function(*args, **kwargs, unknown=None) + + +# +# List of all unit tests that check for unrecognized keywords +# +# Every function that accepts variable keyword arguments (**kwargs) should +# have an entry in this table, to make sure that nothing is missing. This +# will also force people who add new functions to put in an appropriate unit +# test. +# + +kwarg_unittest = { + 'bode': test_matplotlib_kwargs, + 'bode_plot': test_matplotlib_kwargs, + 'describing_function_plot': None, + 'dlqr': statefbk_test.TestStatefbk.test_lqr_errors, + 'drss': test_unrecognized_kwargs, + 'find_eqpt': None, + 'gangof4': test_matplotlib_kwargs, + 'gangof4_plot': test_matplotlib_kwargs, + 'input_output_response': test_unrecognized_kwargs, + 'interconnect': interconnect_test.test_interconnect_exceptions, + 'linearize': None, + 'lqr': statefbk_test.TestStatefbk.test_lqr_errors, + 'nyquist': test_matplotlib_kwargs, + 'nyquist_plot': test_matplotlib_kwargs, + 'pzmap': None, + 'rlocus': test_unrecognized_kwargs, + 'root_locus': test_unrecognized_kwargs, + 'rss': test_unrecognized_kwargs, + 'set_defaults': None, + 'singular_values_plot': None, + 'ss': test_unrecognized_kwargs, + 'ss2io': test_unrecognized_kwargs, + 'ss2tf': test_unrecognized_kwargs, + 'summing_junction': interconnect_test.test_interconnect_exceptions, + 'tf': test_unrecognized_kwargs, + 'tf2io' : test_unrecognized_kwargs, + 'flatsys.point_to_point': + flatsys_test.TestFlatSys.test_point_to_point_errors, + 'FrequencyResponseData.__init__': + frd_test.TestFRD.test_unrecognized_keyword, + 'InputOutputSystem.__init__': None, + 'InputOutputSystem.linearize': None, + 'InterconnectedSystem.__init__': + interconnect_test.test_interconnect_exceptions, + 'InterconnectedSystem.linearize': None, + 'LinearICSystem.linearize': None, + 'LinearIOSystem.__init__': + interconnect_test.test_interconnect_exceptions, + 'LinearIOSystem.linearize': None, + 'NonlinearIOSystem.__init__': + interconnect_test.test_interconnect_exceptions, + 'NonlinearIOSystem.linearize': None, + 'StateSpace.__init__': None, + 'TimeResponseData.__call__': trdata_test.test_response_copy, + 'TransferFunction.__init__': None, + 'flatsys.FlatSystem.linearize': None, + 'flatsys.LinearFlatSystem.linearize': None, +} diff --git a/control/xferfcn.py b/control/xferfcn.py index 76d704afc..a171a1143 100644 --- a/control/xferfcn.py +++ b/control/xferfcn.py @@ -1574,7 +1574,11 @@ def ss2tf(*args, **kwargs): # Assume we were given the A, B, C, D matrix and (optional) dt return _convert_to_transfer_function(StateSpace(*args, **kwargs)) - elif len(args) == 1: + # Make sure there were no extraneous keywords + if kwargs: + raise TypeError("unrecognized keywords: ", str(kwargs)) + + if len(args) == 1: sys = args[0] if isinstance(sys, StateSpace): return _convert_to_transfer_function(sys) From 26303ff99234096ee4833cecf9fceb2ae9090159 Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Sun, 20 Mar 2022 23:06:21 -0700 Subject: [PATCH 7/8] move kwarg lists inside function to fix initialization issue --- control/tests/kwargs_test.py | 100 ++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 48 deletions(-) diff --git a/control/tests/kwargs_test.py b/control/tests/kwargs_test.py index 089d910c8..5275dbd2d 100644 --- a/control/tests/kwargs_test.py +++ b/control/tests/kwargs_test.py @@ -58,54 +58,58 @@ def test_kwarg_search(module, prefix): test_kwarg_search(obj, prefix + obj.__name__ + '.') -# Create a SISO system for use in parameterized tests -sys = control.ss([[-1, 1], [0, -1]], [[0], [1]], [[1, 0]], 0, dt=None) - - -# Parameterized tests for looking for unrecognized keyword errors -@pytest.mark.parametrize("function, args, kwargs", [ - [control.dlqr, (sys, [[1, 0], [0, 1]], [[1]]), {}], - [control.drss, (2, 1, 1), {}], - [control.input_output_response, (sys, [0, 1, 2], [1, 1, 1]), {}], - [control.lqr, (sys, [[1, 0], [0, 1]], [[1]]), {}], - [control.pzmap, (sys,), {}], - [control.rlocus, (control.tf([1], [1, 1]), ), {}], - [control.root_locus, (control.tf([1], [1, 1]), ), {}], - [control.rss, (2, 1, 1), {}], - [control.ss, (0, 0, 0, 0), {'dt': 1}], - [control.ss2io, (sys,), {}], - [control.summing_junction, (2,), {}], - [control.tf, ([1], [1, 1]), {}], - [control.tf2io, (control.tf([1], [1, 1]),), {}], - [control.InputOutputSystem, (1, 1, 1), {}], - [control.StateSpace, ([[-1, 0], [0, -1]], [[1], [1]], [[1, 1]], 0), {}], - [control.TransferFunction, ([1], [1, 1]), {}], -]) -def test_unrecognized_kwargs(function, args, kwargs): - # Call the function normally and make sure it works - function(*args, **kwargs) - - # Now add an unrecognized keyword and make sure there is an error - with pytest.raises(TypeError, match="unrecognized keyword"): - function(*args, **kwargs, unknown=None) - - -# Parameterized tests for looking for keyword errors handled by matplotlib -@pytest.mark.parametrize("function, args, kwargs", [ - [control.bode, (sys, ), {}], - [control.bode_plot, (sys, ), {}], - [control.gangof4, (sys, sys), {}], - [control.gangof4_plot, (sys, sys), {}], - [control.nyquist, (sys, ), {}], - [control.nyquist_plot, (sys, ), {}], -]) -def test_matplotlib_kwargs(function, args, kwargs): - # Call the function normally and make sure it works - function(*args, **kwargs) - - # Now add an unrecognized keyword and make sure there is an error - with pytest.raises(AttributeError, match="has no property"): - function(*args, **kwargs, unknown=None) +def test_unrecognized_kwargs(): + # Create a SISO system for use in parameterized tests + sys = control.ss([[-1, 1], [0, -1]], [[0], [1]], [[1, 0]], 0, dt=None) + + table = [ + [control.dlqr, (sys, [[1, 0], [0, 1]], [[1]]), {}], + [control.drss, (2, 1, 1), {}], + [control.input_output_response, (sys, [0, 1, 2], [1, 1, 1]), {}], + [control.lqr, (sys, [[1, 0], [0, 1]], [[1]]), {}], + [control.pzmap, (sys,), {}], + [control.rlocus, (control.tf([1], [1, 1]), ), {}], + [control.root_locus, (control.tf([1], [1, 1]), ), {}], + [control.rss, (2, 1, 1), {}], + [control.ss, (0, 0, 0, 0), {'dt': 1}], + [control.ss2io, (sys,), {}], + [control.summing_junction, (2,), {}], + [control.tf, ([1], [1, 1]), {}], + [control.tf2io, (control.tf([1], [1, 1]),), {}], + [control.InputOutputSystem, (1, 1, 1), {}], + [control.StateSpace, ([[-1, 0], [0, -1]], [[1], [1]], [[1, 1]], 0), {}], + [control.TransferFunction, ([1], [1, 1]), {}], + ] + + for function, args, kwargs in table: + # Call the function normally and make sure it works + function(*args, **kwargs) + + # Now add an unrecognized keyword and make sure there is an error + with pytest.raises(TypeError, match="unrecognized keyword"): + function(*args, **kwargs, unknown=None) + + +def test_matplotlib_kwargs(): + # Create a SISO system for use in parameterized tests + sys = control.ss([[-1, 1], [0, -1]], [[0], [1]], [[1, 0]], 0, dt=None) + + table = [ + [control.bode, (sys, ), {}], + [control.bode_plot, (sys, ), {}], + [control.gangof4, (sys, sys), {}], + [control.gangof4_plot, (sys, sys), {}], + [control.nyquist, (sys, ), {}], + [control.nyquist_plot, (sys, ), {}], + ] + + for function, args, kwargs in table: + # Call the function normally and make sure it works + function(*args, **kwargs) + + # Now add an unrecognized keyword and make sure there is an error + with pytest.raises(AttributeError, match="has no property"): + function(*args, **kwargs, unknown=None) # From 87cb31a5b4dccdc3541c0c02672e9aaae96ef82c Mon Sep 17 00:00:00 2001 From: Richard Murray Date: Mon, 21 Mar 2022 21:20:08 -0700 Subject: [PATCH 8/8] add missing kwarg tests + small bug fixes --- control/config.py | 5 +- control/iosys.py | 10 ++-- control/tests/config_test.py | 8 ++-- control/tests/kwargs_test.py | 88 ++++++++++++++++++++++-------------- 4 files changed, 68 insertions(+), 43 deletions(-) diff --git a/control/config.py b/control/config.py index 8acdf28e2..605fbcb23 100644 --- a/control/config.py +++ b/control/config.py @@ -73,6 +73,9 @@ def set_defaults(module, **keywords): if not isinstance(module, str): raise ValueError("module must be a string") for key, val in keywords.items(): + keyname = module + '.' + key + if keyname not in defaults and f"deprecated.{keyname}" not in defaults: + raise TypeError(f"unrecognized keyword: {key}") defaults[module + '.' + key] = val @@ -289,6 +292,6 @@ def use_legacy_defaults(version): set_defaults('control', squeeze_time_response=True) # switched mirror_style of nyquist from '-' to '--' - set_defaults('nyqist', mirror_style='-') + set_defaults('nyquist', mirror_style='-') return (major, minor, patch) diff --git a/control/iosys.py b/control/iosys.py index eeb483227..ccfdba2ca 100644 --- a/control/iosys.py +++ b/control/iosys.py @@ -1829,7 +1829,7 @@ def ivp_rhs(t, x): def find_eqpt(sys, x0, u0=[], y0=None, t=0, params={}, iu=None, iy=None, ix=None, idx=None, dx0=None, - return_y=False, return_result=False, **kw): + return_y=False, return_result=False): """Find the equilibrium point for an input/output system. Returns the value of an equilibrium point given the initial state and @@ -1933,7 +1933,7 @@ def find_eqpt(sys, x0, u0=[], y0=None, t=0, params={}, # Take u0 as fixed and minimize over x # TODO: update to allow discrete time systems def ode_rhs(z): return sys._rhs(t, z, u0) - result = root(ode_rhs, x0, **kw) + result = root(ode_rhs, x0) z = (result.x, u0, sys._out(t, result.x, u0)) else: # Take y0 as fixed and minimize over x and u @@ -1944,7 +1944,7 @@ def rootfun(z): return np.concatenate( (sys._rhs(t, x, u), sys._out(t, x, u) - y0), axis=0) z0 = np.concatenate((x0, u0), axis=0) # Put variables together - result = root(rootfun, z0, **kw) # Find the eq point + result = root(rootfun, z0) # Find the eq point x, u = np.split(result.x, [nstates]) # Split result back in two z = (x, u, sys._out(t, x, u)) @@ -2056,7 +2056,7 @@ def rootfun(z): z0 = np.concatenate((x[state_vars], u[input_vars]), axis=0) # Finally, call the root finding function - result = root(rootfun, z0, **kw) + result = root(rootfun, z0) # Extract out the results and insert into x and u x[state_vars] = result.x[:nstate_vars] @@ -2135,7 +2135,7 @@ def _parse_signal_parameter(value, name, kwargs, end=False): if end and kwargs: raise TypeError("unrecognized keywords: ", str(kwargs)) - + return value diff --git a/control/tests/config_test.py b/control/tests/config_test.py index e198254bf..b495a0f6f 100644 --- a/control/tests/config_test.py +++ b/control/tests/config_test.py @@ -23,10 +23,10 @@ class TestConfig: sys = ct.tf([10], [1, 2, 1]) def test_set_defaults(self): - ct.config.set_defaults('config', test1=1, test2=2, test3=None) - assert ct.config.defaults['config.test1'] == 1 - assert ct.config.defaults['config.test2'] == 2 - assert ct.config.defaults['config.test3'] is None + ct.config.set_defaults('freqplot', dB=1, deg=2, Hz=None) + assert ct.config.defaults['freqplot.dB'] == 1 + assert ct.config.defaults['freqplot.deg'] == 2 + assert ct.config.defaults['freqplot.Hz'] is None @mplcleanup def test_get_param(self): diff --git a/control/tests/kwargs_test.py b/control/tests/kwargs_test.py index 5275dbd2d..0502114dc 100644 --- a/control/tests/kwargs_test.py +++ b/control/tests/kwargs_test.py @@ -13,6 +13,7 @@ import inspect import pytest import warnings +import matplotlib.pyplot as plt import control import control.flatsys @@ -36,28 +37,45 @@ def test_kwarg_search(module, prefix): not inspect.getmodule(obj).__name__.startswith('control'): # Skip anything that isn't part of the control package continue - - # Look for functions with keyword arguments - if inspect.isfunction(obj): - # Get the signature for the function - sig = inspect.signature(obj) - - # See if there is a variable keyword argument - for argname, par in sig.parameters.items(): - if par.kind == inspect.Parameter.VAR_KEYWORD: - # Make sure there is a unit test defined - assert prefix + name in kwarg_unittest - - # Make sure there is a unit test - if not hasattr(kwarg_unittest[prefix + name], '__call__'): - warnings.warn("No unit test defined for '%s'" - % prefix + name) + + # Only look for functions with keyword arguments + if not inspect.isfunction(obj): + continue + + # Get the signature for the function + sig = inspect.signature(obj) + + # Skip anything that is inherited + if inspect.isclass(module) and obj.__name__ not in module.__dict__: + continue + + # See if there is a variable keyword argument + for argname, par in sig.parameters.items(): + if not par.kind == inspect.Parameter.VAR_KEYWORD: + continue + + # Make sure there is a unit test defined + assert prefix + name in kwarg_unittest + + # Make sure there is a unit test + if not hasattr(kwarg_unittest[prefix + name], '__call__'): + warnings.warn("No unit test defined for '%s'" % prefix + name) + source = None + else: + source = inspect.getsource(kwarg_unittest[prefix + name]) + + # Make sure the unit test looks for unrecognized keyword + if source and source.find('unrecognized keyword') < 0: + warnings.warn( + f"'unrecognized keyword' not found in unit test " + f"for {name}") # Look for classes and then check member functions if inspect.isclass(obj): test_kwarg_search(obj, prefix + obj.__name__ + '.') +@pytest.mark.usefixtures('editsdefaults') def test_unrecognized_kwargs(): # Create a SISO system for use in parameterized tests sys = control.ss([[-1, 1], [0, -1]], [[0], [1]], [[1, 0]], 0, dt=None) @@ -67,16 +85,20 @@ def test_unrecognized_kwargs(): [control.drss, (2, 1, 1), {}], [control.input_output_response, (sys, [0, 1, 2], [1, 1, 1]), {}], [control.lqr, (sys, [[1, 0], [0, 1]], [[1]]), {}], + [control.linearize, (sys, 0, 0), {}], [control.pzmap, (sys,), {}], [control.rlocus, (control.tf([1], [1, 1]), ), {}], [control.root_locus, (control.tf([1], [1, 1]), ), {}], [control.rss, (2, 1, 1), {}], + [control.set_defaults, ('control',), {'default_dt': True}], [control.ss, (0, 0, 0, 0), {'dt': 1}], [control.ss2io, (sys,), {}], + [control.ss2tf, (sys,), {}], [control.summing_junction, (2,), {}], [control.tf, ([1], [1, 1]), {}], [control.tf2io, (control.tf([1], [1, 1]),), {}], [control.InputOutputSystem, (1, 1, 1), {}], + [control.InputOutputSystem.linearize, (sys, 0, 0), {}], [control.StateSpace, ([[-1, 0], [0, -1]], [[1], [1]], [[1, 1]], 0), {}], [control.TransferFunction, ([1], [1, 1]), {}], ] @@ -97,10 +119,13 @@ def test_matplotlib_kwargs(): table = [ [control.bode, (sys, ), {}], [control.bode_plot, (sys, ), {}], + [control.describing_function_plot, + (sys, control.descfcn.saturation_nonlinearity(1), [1, 2, 3, 4]), {}], [control.gangof4, (sys, sys), {}], [control.gangof4_plot, (sys, sys), {}], [control.nyquist, (sys, ), {}], [control.nyquist_plot, (sys, ), {}], + [control.singular_values_plot, (sys, ), {}], ] for function, args, kwargs in table: @@ -110,7 +135,11 @@ def test_matplotlib_kwargs(): # Now add an unrecognized keyword and make sure there is an error with pytest.raises(AttributeError, match="has no property"): function(*args, **kwargs, unknown=None) - + + # If we opened any figures, close them + if plt.gca(): + plt.close('all') + # # List of all unit tests that check for unrecognized keywords @@ -124,24 +153,23 @@ def test_matplotlib_kwargs(): kwarg_unittest = { 'bode': test_matplotlib_kwargs, 'bode_plot': test_matplotlib_kwargs, - 'describing_function_plot': None, + 'describing_function_plot': test_matplotlib_kwargs, 'dlqr': statefbk_test.TestStatefbk.test_lqr_errors, 'drss': test_unrecognized_kwargs, - 'find_eqpt': None, 'gangof4': test_matplotlib_kwargs, 'gangof4_plot': test_matplotlib_kwargs, 'input_output_response': test_unrecognized_kwargs, 'interconnect': interconnect_test.test_interconnect_exceptions, - 'linearize': None, + 'linearize': test_unrecognized_kwargs, 'lqr': statefbk_test.TestStatefbk.test_lqr_errors, 'nyquist': test_matplotlib_kwargs, 'nyquist_plot': test_matplotlib_kwargs, - 'pzmap': None, + 'pzmap': test_matplotlib_kwargs, 'rlocus': test_unrecognized_kwargs, 'root_locus': test_unrecognized_kwargs, 'rss': test_unrecognized_kwargs, - 'set_defaults': None, - 'singular_values_plot': None, + 'set_defaults': test_unrecognized_kwargs, + 'singular_values_plot': test_matplotlib_kwargs, 'ss': test_unrecognized_kwargs, 'ss2io': test_unrecognized_kwargs, 'ss2tf': test_unrecognized_kwargs, @@ -152,21 +180,15 @@ def test_matplotlib_kwargs(): flatsys_test.TestFlatSys.test_point_to_point_errors, 'FrequencyResponseData.__init__': frd_test.TestFRD.test_unrecognized_keyword, - 'InputOutputSystem.__init__': None, - 'InputOutputSystem.linearize': None, + 'InputOutputSystem.__init__': test_unrecognized_kwargs, + 'InputOutputSystem.linearize': test_unrecognized_kwargs, 'InterconnectedSystem.__init__': interconnect_test.test_interconnect_exceptions, - 'InterconnectedSystem.linearize': None, - 'LinearICSystem.linearize': None, 'LinearIOSystem.__init__': interconnect_test.test_interconnect_exceptions, - 'LinearIOSystem.linearize': None, 'NonlinearIOSystem.__init__': interconnect_test.test_interconnect_exceptions, - 'NonlinearIOSystem.linearize': None, - 'StateSpace.__init__': None, + 'StateSpace.__init__': test_unrecognized_kwargs, 'TimeResponseData.__call__': trdata_test.test_response_copy, - 'TransferFunction.__init__': None, - 'flatsys.FlatSystem.linearize': None, - 'flatsys.LinearFlatSystem.linearize': None, + 'TransferFunction.__init__': test_unrecognized_kwargs, }