Improve markov function, add mimo support, change api to TimeResponseData by KybernetikJo · Pull Request #1022 · python-control/python-control

KybernetikJo · 2024-07-03T17:57:58Z

This PR adds

MIMO support to the markov function and
it uses the TimeResponseData Type.

T = np.linspace(0, 10, 100)
U = np.ones((1, 100))
response = ct.forced_response(ct.tf([1], [1, 0.5], True), T, U)
H = ct.markov(response, 3)

~~This breaks old user code!~~
Does not break old user code!

T = np.linspace(0, 10, 100)
U = np.ones((1, 100))
T, Y = ct.forced_response(ct.tf([1], [1, 0.5], True), T, U)
H = ct.markov(Y, U, 3, transpose=False)

coveralls · 2024-07-03T18:10:38Z

coverage: 94.519% (-0.01%) from 94.533%
when pulling ec38f2e on KybernetikJo:improve_markove_mimo
into 4acc78b on python-control:main.

coveralls · 2024-07-04T15:57:38Z

coverage: 94.519% (-0.01%) from 94.533%
when pulling 4c1f4ca on KybernetikJo:improve_markove_mimo
into 4acc78b on python-control:main.

coveralls · 2024-07-04T16:32:53Z

coverage: 94.519% (-0.01%) from 94.533%
when pulling 446a161 on KybernetikJo:improve_markove_mimo
into 4acc78b on python-control:main.

murrayrm · 2024-07-04T20:05:53Z

Rather than breaking old code, it would be nice to do this in a way that preserved prior functionality. So use *args processing to allow both of the following to work:

H = ct.markov(response, 3)
H = ct.markov(Y, U, 3)

It also seems a bit odd (to me) to have the return type be a TimeResponseData object. It true that the Markov parameters are the impulse response, but are people going to be plotting that impulse response as the primary way they interact with the output of the markov command?

KybernetikJo · 2024-07-04T22:00:17Z

Rather than breaking old code, it would be nice to do this in a way that preserved prior functionality. So use *args processing to allow both of the following to work:
H = ct.markov(response, 3)
H = ct.markov(Y, U, 3)

I will update this one.

It also seems a bit odd (to me) to have the return type be a TimeResponseData object. It true that the Markov parameters are the impulse response, but are people going to be plotting that impulse response as the primary way they interact with the output of the markov command?

I do not have strong feelings about that. The idea was to simplify the joint use of markov and era:

impulse_response = ct.markov(response, m=10)
sysd = ct.era(impulse_response, r=4)

Direct access of Markov parameters:

_, H = ct.markov(response, m=10)

I can change the return value to an array. I think it is also better for the old API.
I guess I should also keep the old API for era.

H = ct.markov(Y, U, m=10)
sysd = ct.era(H, r=4)

coveralls · 2024-07-07T11:53:03Z

coverage: 94.538% (+0.005%) from 94.533%
when pulling 317d261 on KybernetikJo:improve_markove_mimo
into 4acc78b on python-control:main.

coveralls · 2024-07-07T14:20:37Z

coverage: 94.516% (-0.02%) from 94.533%
when pulling 2b1cd21 on KybernetikJo:improve_markove_mimo
into 4acc78b on python-control:main.

coveralls · 2024-07-07T14:35:28Z

coverage: 94.516% (-0.02%) from 94.533%
when pulling cee59d1 on KybernetikJo:improve_markove_mimo
into 4acc78b on python-control:main.

…Data

coveralls · 2024-07-11T13:24:53Z

coverage: 94.669% (+0.04%) from 94.629%
when pulling e61ac53 on KybernetikJo:improve_markove_mimo
into da64e0e on python-control:main.

murrayrm

Overall this looks OK. It would be good to add unit tests to cover some of the lines that are not currently covered.

murrayrm · 2024-07-13T18:06:44Z

control/modelsimp.py

+    """markov(Y, U, [, m])
+
+    Calculate the first `m` Markov parameters [D CB CAB ...]
+    from data


Should end in a period.

murrayrm · 2024-07-13T18:07:56Z

control/modelsimp.py

-    if Umat.shape[0] != 1 or Ymat.shape[0] != 1:
-        raise ControlMIMONotImplemented
+    # Get the system description
+    if (len(args) < 1):


Parenthesis are not needed here. The more standard style convention would be

if len(args) < 1:

murrayrm · 2024-07-13T18:08:21Z

control/modelsimp.py

+        elif (len(args) > 2):
+            raise ControlArgument("too many positional arguments")
+    else:
+        if (len(args) < 2):


OK to remove parens (here and below).

murrayrm · 2024-07-13T18:11:59Z

control/modelsimp.py

+    dt : True of float, optional
+        True indicates discrete time with unspecified sampling time,
+        positive number is discrete time with specified sampling time.It
+        can be used to scale the markov parameters in order to match the


Markov should be capitalized.

murrayrm · 2024-07-13T18:15:16Z

control/modelsimp.py

+    dt : True of float, optional
+        True indicates discrete time with unspecified sampling time,
+        positive number is discrete time with specified sampling time.It
+        can be used to scale the markov parameters in order to match the


In the case that a TimeResponseData object is given, should dt default to the value coming from the time array? You could set the default value of dt to None and then if the input is Y, U then you default to dt=1 and the input is data, you default to data.time[1] - data.time[0] (perhaps with a check to make sure that time points are equally spaced?).

…onse time data

KybernetikJo marked this pull request as draft July 4, 2024 12:03

KybernetikJo added 10 commits July 11, 2024 15:08

Improve markov function, add mimo support, change api to TimeResponse…

f2f0e3e

…Data

Fix inputs dimension

6c95fbc

Add plot_inputs=False to TimeResponseData output

14ce769

Add markov example

efa3f39

Add markov example, add example to doc

a597bbc

Change output to ndarray

6023480

Update example, values taken from a book

92307f0

Change API to work with ndarray and TimeResponseData as input

a646100

Update pytest

42962f2

Refactor api, keep old api working

a3276b7

KybernetikJo force-pushed the improve_markove_mimo branch from cee59d1 to a3276b7 Compare July 11, 2024 13:11

KybernetikJo marked this pull request as ready for review July 11, 2024 13:25

murrayrm requested changes Jul 13, 2024

View reviewed changes

KybernetikJo mentioned this pull request Jul 15, 2024

Implement ERA, change api to TimeResponseData #1024

Merged

KybernetikJo added 3 commits July 15, 2024 11:20

Remove parenthesis form if, update docstrings and comments

7ac6ee0

Change default value of dt to None, add equally spaced check for resp…

8d62775

…onse time data

Add unit tests to increase code coverage

e61ac53

KybernetikJo force-pushed the improve_markove_mimo branch from fd87d92 to e61ac53 Compare July 15, 2024 11:32

KybernetikJo requested a review from murrayrm July 16, 2024 10:00

murrayrm merged commit 9809a78 into python-control:main Aug 6, 2024

murrayrm added this to the 0.10.1 milestone Aug 8, 2024

slivingston mentioned this pull request Aug 8, 2024

Control plot refactoring for consistent functionality #1034

Merged

Comments

Conversation

KybernetikJo commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 3, 2024

Uh oh!

coveralls commented Jul 4, 2024

Uh oh!

coveralls commented Jul 4, 2024

Uh oh!

murrayrm commented Jul 4, 2024

Uh oh!

KybernetikJo commented Jul 4, 2024

Uh oh!

coveralls commented Jul 7, 2024

Uh oh!

coveralls commented Jul 7, 2024

Uh oh!

coveralls commented Jul 7, 2024

Uh oh!

coveralls commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KybernetikJo commented Jul 3, 2024 •

edited

Loading

coveralls commented Jul 11, 2024 •

edited

Loading