NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

Improve markov function, add mimo support, change api to TimeResponseData#1022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

murrayrm merged 13 commits intopython-control:mainfromKybernetikJo:improve_markove_mimo

Aug 6, 2024

Merged

Improve markov function, add mimo support, change api to TimeResponseData#1022

murrayrm merged 13 commits intopython-control:mainfromKybernetikJo:improve_markove_mimo

Aug 6, 2024

Conversation

Copy link

Contributor

KybernetikJo commentedJul 3, 2024•
edited
Loading

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)

Copy link

coveralls commentedJul 3, 2024

coverage: 94.519% (-0.01%) from 94.533%
when pullingec38f2e on KybernetikJo:improve_markove_mimo
into4acc78b on python-control:main.

KybernetikJo marked this pull request as draft

July 4, 2024 12:03

Copy link

coveralls commentedJul 4, 2024

coverage: 94.519% (-0.01%) from 94.533%
when pulling4c1f4ca on KybernetikJo:improve_markove_mimo
into4acc78b on python-control:main.

Copy link

coveralls commentedJul 4, 2024

coverage: 94.519% (-0.01%) from 94.533%
when pulling446a161 on KybernetikJo:improve_markove_mimo
into4acc78b on python-control:main.

Copy link

Member

murrayrm commentedJul 4, 2024

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 aTimeResponseData 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 themarkov command?

Copy link

ContributorAuthor

KybernetikJo commentedJul 4, 2024

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 aTimeResponseData 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 themarkov 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)

Copy link

coveralls commentedJul 7, 2024

coverage: 94.538% (+0.005%) from 94.533%
when pulling317d261 on KybernetikJo:improve_markove_mimo
into4acc78b on python-control:main.

Copy link

coveralls commentedJul 7, 2024

coverage: 94.516% (-0.02%) from 94.533%
when pulling2b1cd21 on KybernetikJo:improve_markove_mimo
into4acc78b on python-control:main.

Copy link

coveralls commentedJul 7, 2024

coverage: 94.516% (-0.02%) from 94.533%
when pullingcee59d1 on KybernetikJo:improve_markove_mimo
into4acc78b on python-control:main.

KybernetikJo added10 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 theimprove_markove_mimo branch fromcee59d1 toa3276b7Compare

July 11, 2024 13:11

Copy link

coveralls commentedJul 11, 2024•
edited
Loading

coverage: 94.669% (+0.04%) from 94.629%
when pullinge61ac53 on KybernetikJo:improve_markove_mimo
intoda64e0e on python-control:main.

KybernetikJo marked this pull request as ready for review

July 11, 2024 13:25

murrayrm requested changes

Jul 13, 2024

View reviewed changes

Copy link

Member

murrayrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

control/modelsimp.py Outdated

		"""markov(Y, U, [, m])

		Calculate the first `m` Markov parameters [D CB CAB ...]
		from data

Copy link

Member

murrayrmJul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should end in a period.

Copy link

ContributorAuthor

KybernetikJoJul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

control/modelsimp.py Outdated

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

Copy link

Member

murrayrmJul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

if len(args) < 1:

Copy link

ContributorAuthor

KybernetikJoJul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

control/modelsimp.py Outdated

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

Copy link

Member

murrayrmJul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

OK to remove parens (here and below).

Copy link

ContributorAuthor

KybernetikJoJul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

control/modelsimp.py Outdated

		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

Copy link

Member

murrayrmJul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Markov should be capitalized.

Copy link

ContributorAuthor

KybernetikJoJul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

control/modelsimp.py Outdated

Comment on lines 447 to 450

		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

Copy link

Member

murrayrmJul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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