GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Improve markov function, add mimo support, change api to TimeResponseData by KybernetikJo · Pull Request #1022 · python-control/python-control

Conversation

@KybernetikJo

@KybernetikJo KybernetikJo commented Jul 3, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@coveralls

coveralls commented Jul 3, 2024

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

coveralls commented Jul 4, 2024

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coveralls

coveralls commented Jul 4, 2024

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@murrayrm

murrayrm commented Jul 4, 2024

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KybernetikJo

KybernetikJo commented Jul 4, 2024

Copy link
Copy Markdown
Contributor Author

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coveralls

coveralls commented Jul 7, 2024

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coveralls

coveralls commented Jul 7, 2024

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coveralls

coveralls commented Jul 7, 2024

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

KybernetikJo added 10 commits July 11, 2024 15:08
@KybernetikJo
Improve markov function, add mimo support, change api to TimeResponse…
f2f0e3e
…Data
@KybernetikJo
Fix inputs dimension
6c95fbc
@KybernetikJo
Add plot_inputs=False to TimeResponseData output
14ce769
@KybernetikJo
Add markov example
efa3f39
@KybernetikJo
Add markov example, add example to doc
a597bbc
@KybernetikJo
Change output to ndarray
6023480
@KybernetikJo
Update example, values taken from a book
92307f0
@KybernetikJo
Change API to work with ndarray and TimeResponseData as input
a646100
@KybernetikJo
Update pytest
42962f2
@KybernetikJo
Refactor api, keep old api working
a3276b7
@KybernetikJo
KybernetikJo force-pushed the improve_markove_mimo branch from cee59d1 to a3276b7 Compare July 11, 2024 13:11
@coveralls

coveralls commented Jul 11, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KybernetikJo
KybernetikJo marked this pull request as ready for review July 11, 2024 13:25
murrayrm
murrayrm requested changes Jul 13, 2024
View reviewed changes

@murrayrm murrayrm left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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 the lines that are not currently covered.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread control/modelsimp.py Outdated
"""markov(Y, U, [, m])

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

@murrayrm murrayrm Jul 13, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@KybernetikJo KybernetikJo Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread control/modelsimp.py Outdated
if Umat.shape[0] != 1 or Ymat.shape[0] != 1:
raise ControlMIMONotImplemented
# Get the system description
if (len(args) < 1):

@murrayrm murrayrm Jul 13, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@KybernetikJo KybernetikJo Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread control/modelsimp.py Outdated
elif (len(args) > 2):
raise ControlArgument("too many positional arguments")
else:
if (len(args) < 2):

@murrayrm murrayrm Jul 13, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@KybernetikJo KybernetikJo Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread 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

@murrayrm murrayrm Jul 13, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Markov should be capitalized.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@KybernetikJo KybernetikJo Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread 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

@murrayrm murrayrm Jul 13, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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, 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?).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@KybernetikJo KybernetikJo Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@KybernetikJo 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
@KybernetikJo
Remove parenthesis form if, update docstrings and comments
7ac6ee0
@KybernetikJo
Change default value of dt to None, add equally spaced check for resp…
8d62775
…onse time data
@KybernetikJo
Add unit tests to increase code coverage
e61ac53
@KybernetikJo
KybernetikJo force-pushed the improve_markove_mimo branch from fd87d92 to e61ac53 Compare July 15, 2024 11:32
@KybernetikJo
KybernetikJo requested a review from murrayrm July 16, 2024 10:00
@murrayrm
murrayrm merged commit 9809a78 into python-control:main Aug 6, 2024
@murrayrm murrayrm added this to the 0.10.1 milestone Aug 8, 2024
@slivingston slivingston mentioned this pull request Aug 8, 2024
Control plot refactoring for consistent functionality #1034
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murrayrm murrayrm Awaiting requested review from murrayrm

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.10.1

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@KybernetikJo @coveralls @murrayrm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.