Repository: python-control/python-control
Pull request #1064 (2024-11-23 02:57:41)

fix timebase processing in frd, zpk
This PR addresses issue #1062 by making sure that timebases are handled uniformly in all system creation factory functions:
* For `ss`, `tf`, `frd`, and `zpk`, use either an additional argument specifying the sampling time or use the `dt` keyword (using both generates a warning).
* For `nlsys`, use the `dt` keyword
* If no timebase is specified, use the default timebase set by `config.defaults['control.default_dt']`, which is 0 (continuous time) unless you change it.
It was the last of these that was the problem in `zpk`: it was defaulting to `None` and that was later interpreted as a discrete time system.

Also added unit tests to make sure everything is consistent across the different factory functions.




Patch:
diff --git a/control/frdata.py b/control/frdata.py
index c5018bab..42ecee0d 100644
--- a/control/frdata.py
+++ b/control/frdata.py
@@ -124,13 +124,24 @@ class FrequencyResponseData(LTI):
         To construct frequency response data for an existing LTI
         object, other than an FRD, call FRD(sys, omega).
 
+        The timebase for the frequency response can be provided using an
+        optional third argument or the 'dt' keyword.
+
         """
-        # TODO: discrete-time FRD systems?
         smooth = kwargs.pop('smooth', False)
 
         #
         # Process positional arguments
         #
+        if len(args) == 3:
+            # Discrete time transfer function
+            dt = args[-1]
+            if 'dt' in kwargs:
+                warn("received multiple dt arguments, "
+                     "using positional arg dt = %s" % dt)
+            kwargs['dt'] = dt
+            args = args[:-1]
+
         if len(args) == 2:
             if not isinstance(args[0], FRD) and isinstance(args[0], LTI):
                 # not an FRD, but still a system, second argument should be
@@ -200,11 +211,11 @@ class FrequencyResponseData(LTI):
 
         # Process iosys keywords
         defaults = {
-            'inputs': self.fresp.shape[1], 'outputs': self.fresp.shape[0],
-            'dt': None}
+            'inputs': self.fresp.shape[1], 'outputs': self.fresp.shape[0]}
+        if arg_dt is not None:
+            defaults['dt'] = arg_dt             # choose compatible timebase
         name, inputs, outputs, states, dt = _process_iosys_keywords(
                 kwargs, defaults, end=True)
-        dt = common_timebase(dt, arg_dt)        # choose compatible timebase
 
         # Process signal names
         InputOutputSystem.__init__(
diff --git a/control/xferfcn.py b/control/xferfcn.py
index ee41cbd2..499359cb 100644
--- a/control/xferfcn.py
+++ b/control/xferfcn.py
@@ -1677,7 +1677,7 @@ def tf(*args, **kwargs):
         raise ValueError("Needs 1 or 2 arguments; received %i." % len(args))
 
 
-def zpk(zeros, poles, gain, dt=None, **kwargs):
+def zpk(zeros, poles, gain, *args, **kwargs):
     """zpk(zeros, poles, gain[, dt])
 
     Create a transfer function from zeros, poles, gain.
@@ -1732,7 +1732,7 @@ def zpk(zeros, poles, gain, dt=None, **kwargs):
 
     """
     num, den = zpk2tf(zeros, poles, gain)
-    return TransferFunction(num, den, dt=dt, **kwargs)
+    return TransferFunction(num, den, *args, **kwargs)
 
 
 def ss2tf(*args, **kwargs):


Test patch:
diff --git a/control/tests/docstrings_test.py b/control/tests/docstrings_test.py
index 2b1368ae..27ced105 100644
--- a/control/tests/docstrings_test.py
+++ b/control/tests/docstrings_test.py
@@ -54,6 +54,7 @@ keyword_skiplist = {
     control.bode_plot: ['sharex', 'sharey', 'margin_info'], # deprecated
     control.eigensys_realization: ['arg'],                  # quasi-positional
     control.find_operating_point: ['method'],               # internal use
+    control.zpk: ['args']                                   # 'dt' (manual)
 }
 
 # Decide on the level of verbosity (use -rP when running pytest)
diff --git a/control/tests/timebase_test.py b/control/tests/timebase_test.py
index 79b1492d..c0e02d3b 100644
--- a/control/tests/timebase_test.py
+++ b/control/tests/timebase_test.py
@@ -97,3 +97,34 @@ def test_composition_override(dt):
         with pytest.raises(ValueError, match="incompatible timebases"):
             sys3 = ct.interconnect(
                 [sys1, sys2], inputs='u1', outputs='y2', dt=dt)
+
+
+# Make sure all system creation functions treat timebases uniformly
+@pytest.mark.parametrize(
+    "fcn, args", [
+        (ct.ss, [-1, 1, 1, 1]),
+        (ct.tf, [[1, 2], [3, 4, 5]]),
+        (ct.zpk, [[-1], [-2, -3], 1]),
+        (ct.frd, [[1, 1, 1], [1, 2, 3]]),
+        (ct.nlsys, [lambda t, x, u, params: -x, None]),
+    ])
+@pytest.mark.parametrize(
+    "kwargs, expected", [
+        ({}, 0),
+        ({'dt': 0}, 0),
+        ({'dt': 0.1}, 0.1),
+        ({'dt': True}, True),
+        ({'dt': None}, None),
+    ])
+def test_default(fcn, args, kwargs, expected):
+    sys = fcn(*args, **kwargs)
+    assert sys.dt == expected
+
+    # Some commands allow dt via extra argument
+    if fcn in [ct.ss, ct.tf, ct.zpk, ct.frd] and kwargs.get('dt'):
+        sys = fcn(*args, kwargs['dt'])
+        assert sys.dt == expected
+
+        # Make sure an error is generated if dt is redundant
+        with pytest.warns(UserWarning, match="received multiple dt"):
+            sys = fcn(*args, kwargs['dt'], **kwargs)