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 — Implement ERA, change api to TimeResponseData by KybernetikJo · Pull Request #1024 · python-control/python-control

Conversation

@KybernetikJo

@KybernetikJo KybernetikJo commented Jul 4, 2024

Copy link
Copy Markdown
Contributor

This PR implements the ERA method. Related to #1002 and #847

  • it uses the TimeResponseData Type.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@coveralls

coveralls commented Jul 4, 2024

Copy link
Copy Markdown

Coverage Status

coverage: 94.21% (-0.3%) from 94.533%
when pulling 28744f8 on KybernetikJo:implement_era
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.21% (-0.3%) from 94.533%
when pulling 596e5ed on KybernetikJo:implement_era
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.374% (-0.2%) from 94.533%
when pulling 55ac0ed on KybernetikJo:implement_era
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 5 commits July 7, 2024 19:31
@KybernetikJo
Implement ERA, change api to TimeResponseData
cd87f2f
@KybernetikJo
Add era example
8a9c497
@KybernetikJo
Rename era example file name, small clean up
562824c
@KybernetikJo
Add era example to doc
6bbad5f
@KybernetikJo
Change API to work with TimeResponseData and ndarray as input, add py…
614a080 
…test, fix small things
@KybernetikJo
KybernetikJo force-pushed the implement_era branch from 55ac0ed to 614a080 Compare July 7, 2024 17:32
@coveralls

coveralls commented Jul 7, 2024

Copy link
Copy Markdown

Coverage Status

coverage: 94.533% (-0.02%) from 94.554%
when pulling 614a080 on KybernetikJo:implement_era
into 7a135d1 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 7, 2024 17:46
murrayrm
murrayrm requested changes Jul 9, 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 good. I found a few small things that you might want to update.

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 +397 to +398
impulse-response data from which the StateSpace model is estimated,
1D or 3D array.

@murrayrm murrayrm Jul 9, 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.

impulse → Impulse (start sentence with a capital letter) and try to fit on one line (if less than 75 characters).

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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
impulse-response data from which the StateSpace model is estimated,
1D or 3D array.
data : TimeResponseData
impulse-response data from which the StateSpace model is estimated.

@murrayrm murrayrm Jul 9, 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.

impose → Impulse

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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 +404 to +405
Number of rows in Hankel matrix.
Default is 2*r.

@murrayrm murrayrm Jul 9, 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.

Put on a single line (here and below).

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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 +410 to +414
True indicates discrete time with unspecified sampling time,
positive number is discrete time with specified sampling time.
It can be used to scale the StateSpace model in order to match
the impulse response of this library.
Default values is True.

@murrayrm murrayrm Jul 9, 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.

Can probably have fewer lines by going out to column 75 before wrapping. Default value sentence doesn't need to start on a new line.

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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.

Tried and 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 +372 to +373
def era(arg, r, m=None, n=None, dt=True, transpose=False):
r"""era(YY, r)

@murrayrm murrayrm Jul 9, 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.

Optional: to be consistent with the longer naming style we are using in python-control, I suggest we call the function eigensys_realization and then define era = eigensys_realization at the bottom of the file (see freqplot.py for some example). We should also (but not here) redefine the other functions in this file (minimal_realization, balanced_realization, etc).

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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! See #1028

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@KybernetikJo
KybernetikJo requested a review from murrayrm July 11, 2024 08:57
@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.608% (-0.02%) from 94.628%
when pulling 8b93264 on KybernetikJo:implement_era
into 6406868 on python-control:main.
All reactions

Sorry, something went wrong.

Uh oh!

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

KybernetikJo commented Jul 11, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I get a warning

../python-control/doc/generated/control.era.rst: WARNING: document isn't included in any toctree

I could not exclude the alias name era from autosummary.

__all__=[eigensys_realization, era]

How to exclude era from autosummary?

SOLUTION: The Solution is, to delete the generated folder in doc. Then the sphinx/autosummary does not use old files.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KybernetikJo
KybernetikJo force-pushed the implement_era branch from c5f0cbe to 4ea1d82 Compare July 11, 2024 11:24
@KybernetikJo
Fix few docstring things, change name to eigensys_realization
250c448
@KybernetikJo
KybernetikJo force-pushed the implement_era branch from 4ea1d82 to 250c448 Compare July 11, 2024 14:04
@KybernetikJo KybernetikJo mentioned this pull request Jul 11, 2024
Change function names in modelsimp, add aliases #1028
Merged
roryyorke
roryyorke reviewed Jul 14, 2024
View reviewed changes

@roryyorke roryyorke left a comment

Copy link
Copy Markdown
Contributor

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.

some minor suggestions, and a question.

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 +392 to +393
where `response` is an `TimeResponseData` object, and `YY` is 1D or 3D
array and r is an integer.

@roryyorke roryyorke Jul 14, 2024

Copy link
Copy Markdown
Contributor

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.

Suggested change
where `response` is an `TimeResponseData` object, and `YY` is 1D or 3D
array and r is an integer.
where `data` is a `TimeResponseData` object, `YY` is a 1D or 3D
array, and r is an integer.

response is more specific than data, perhaps better?

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 14, 2024 •
edited
Loading

Uh oh!

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

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.

response or data? Not sure.

For me, the function time_response_plot which uses data for time_response, is the blueprint.

The plotting functions use data for input.
bode_plot

The response data types use response for input.
NyquistResponseData

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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 Jul 15, 2024 •
edited
Loading

Uh oh!

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

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.

OK, in my PR #1022 I initially used response instead of data. I will go with data for now.

@roryyorke @murrayrm what should we use?

Sorry, something went wrong.

Uh oh!

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

@roryyorke roryyorke Jul 20, 2024

Copy link
Copy Markdown
Contributor

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.

I prefer response as it's more specific, but the main thing is to be consistent within the ERA docstring(s).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/modelsimp.py Outdated
Number of columns in Hankel matrix. Default is 2*r.
dt : True or float, optional
True indicates discrete time with unspecified sampling time,
positive number is discrete time with specified sampling time. It

@roryyorke roryyorke Jul 14, 2024

Copy link
Copy Markdown
Contributor

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.

Suggested change
positive number is discrete time with specified sampling time. It
and a positive float means discrete time with the specified sampling time. It

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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
True indicates discrete time with unspecified sampling time,
positive number is discrete time with specified sampling time. It
can be used to scale the StateSpace model in order to match the
impulse response of this library. Default is True.

@roryyorke roryyorke Jul 14, 2024

Copy link
Copy Markdown
Contributor

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.

I don't understand the idea of "impulse response of this library" - what does library mean here?

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 14, 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.

This library "python-control" uses a unit area impulse "delta = 1/dt" for dt > 0 and a unit impulse "delta = 1" for dt = True.

The dt parameter is used to create a discrete time state space system (A,B,C,D,dt) to match an impulse response generated by control.impulse_response.

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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!

I change the text to " It can be used to scale the StateSpace model in order to match the unit-area impulse response of python-control. "

Is there a better way to explain that behavior. Should we add something to Library conventions in order to explain the unit-area impulse response of python-control?

Sorry, something went wrong.

Uh oh!

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

@roryyorke roryyorke Jul 20, 2024

Copy link
Copy Markdown
Contributor

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.

Ah, thanks, now I understand.

One possibly-not-obvious side-effect of using True is that it has a value of 1, so one gets debatably natural discrete-time behaviour in that case.

The unit-area convention is documented in timeresp.impulse_resp under Notes, I suggest a reference to that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/modelsimp.py Outdated
>>> sysd, _ = ct.eigensys_realization(response, r=1)
"""
raise NotImplementedError('This function is not implemented yet.')
def block_hankel_matrix(Y, m, n):

@roryyorke roryyorke Jul 14, 2024

Copy link
Copy Markdown
Contributor

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.

suggest making this top-level _block_hankel.

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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
A reduced order model sys=StateSpace(Ar,Br,Cr,Dr,dt).
S : array
Singular values of Hankel matrix. Can be used to choose a good r
value.

@roryyorke roryyorke Jul 14, 2024

Copy link
Copy Markdown
Contributor

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.

given this, is there an alternative interface with a minsigma argument which could be used to choose r based on the singular values instead? (or maybe tol, with minsigma = tol * maxsigma ?)

This probably a bit out-of-scope, and if the user needs this capability it is simple enough to implement in their own code.

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 14, 2024 •
edited
Loading

Uh oh!

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

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.

I have thought about it, but I will not be adding this feature at this time.

I don't know the literature on this topic well enough, but I know that there are papers that study the choice of r.

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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.

Postponed. Possible improvement in the future.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/modelsimp.py Outdated
# balanced realizations
Sigma_inv = np.diag(1./np.sqrt(S[0:r]))
Ar = Sigma_inv @ Ur.T @ Hl @ Vhr.T @ Sigma_inv
Br = Sigma_inv @ Ur.T @ Hf[:,0:p]*dt

@roryyorke roryyorke Jul 14, 2024

Copy link
Copy Markdown
Contributor

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.

It looks like the reference here is "Algorithm 1" of the arxiv paper.

Why does dt makes an appearance here? I don't see any mention of sample rate or sample period in the referenced paper. It must be needed since you have dt=0.1 in the spring-damper model unit test -- I guess I'm missing something obvious?

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 14, 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.

ERA in the paper assumes a unit impulse response "delta = 1". There is no dt in the paper.

However, python control generates a unit-area impulse response "delta = 1/dt". See also: #812, #977.

In other words, the impulse responses of StateSpace(A,B,C,D,dt=True) and StateSpace(A,B,C,D,dt=0.1) differ by a scaling.
The dt parameter in era can be used to create the right StateSpace model.

Alternative:
We could also rescale the output data by Y=Y*dt in order to get the unit impulse response instead of the unit-area impulse response.

Overall, my goal is that impulse_response, era, markov and StateSpace should work well together.

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Jul 15, 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. I added a little comment to indicate the intentional use of Br*dt.

Is there a more elegant way to achieve this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
KybernetikJo added 2 commits July 15, 2024 09:26
@KybernetikJo
Change _block_hankel to top-level, update docstrings and comments
6c73062
@KybernetikJo
Delete the hyphen from the Impuse response.
8b93264
@KybernetikJo
KybernetikJo requested a review from roryyorke July 15, 2024 08:07
roryyorke
roryyorke approved these changes Jul 20, 2024
View reviewed changes

@roryyorke roryyorke left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM. If you think it's useful, add a reference to timeresp.impulse_response.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm
murrayrm merged commit 4c7977b into python-control:main Aug 6, 2024
@murrayrm murrayrm added this to the 0.10.1 milestone Aug 8, 2024
@murrayrm murrayrm linked an issue Nov 23, 2024 that may be closed by this pull request
How to convert finite step response model to state space representation? #1002
Open
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
1 more reviewer
@roryyorke roryyorke roryyorke approved these changes
Reviewers whose approvals may not affect merge requirements

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.

How to convert finite step response model to state space representation?

Uh oh!

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

4 participants

@KybernetikJo @coveralls @roryyorke @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.