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 okid by KybernetikJo · Pull Request #1031 · python-control/python-control

Conversation

@KybernetikJo

@KybernetikJo KybernetikJo commented Jul 15, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This PR implements okid=observer_kalman_identification.

The api should be the same or very similar to that of ct.markov.
An additional goal is for ct.okid and ct.era to work well together.

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.okid(Y, U, 3)
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.okid(response, 3)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@KybernetikJo
KybernetikJo force-pushed the implement_okid branch from 8b337f0 to ae2aea7 Compare July 15, 2024 18:05
@coveralls

coveralls commented Jul 15, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

coverage: 94.127% (-0.6%) from 94.752%
when pulling 5abbfe1 on KybernetikJo:implement_okid
into f73e893 on python-control:main.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KybernetikJo
KybernetikJo force-pushed the implement_okid branch 2 times, most recently from 860acb7 to 67d6ef0 Compare July 16, 2024 09:47
@murrayrm

murrayrm commented Aug 6, 2024

Copy link
Copy Markdown
Member

@KybernetikJo This PR needs to be rebased on main (and okid should be expanded to observer_kalman_identification (or observer_kalman_id to keep things a bit shorter?).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
murrayrm
murrayrm reviewed Aug 6, 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. Some stylistic issues that it would be useful to update for consistency with standard Python style. Perhaps also say something here (or in the markov command) about the relationship between okid and markov?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/modelsimp.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/modelsimp.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/modelsimp.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread examples/okid_msd.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/modelsimp.py
-------
H : ndarray
First m Markov parameters, [D CB CAB ...].

@murrayrm murrayrm Aug 6, 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 we add a "See Also" section and reference markov? Perhaps also a note here (and in markov) about how this differs from the markov command?

Sorry, something went wrong.

Uh oh!

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

@KybernetikJo KybernetikJo Oct 17, 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.

Would that be enough?

See Also

markov

Notes

The :func:~control.markov command estimates the Markov parameters directly, which can be hard for slightly damped systems.
The :func:~control.observer_kalman_identification command uses a Kalman filter, which is better suited for slightly damped systems.

Sorry, something went wrong.

Uh oh!

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

murrayrm commented Aug 13, 2024

Copy link
Copy Markdown
Member

@KybernetikJo I will be doing a release of python-control in the coming days. If you have time to update this PR prior to that, we can include in v0.10.1. Otherwise, it can go in the next release.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KybernetikJo
KybernetikJo force-pushed the implement_okid branch from 46eff1f to bf5888d Compare October 17, 2024 10:13
@KybernetikJo

KybernetikJo commented Oct 19, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@murrayrm

@KybernetikJo I will be doing a release of python-control in the coming days. If you have time to update this PR prior to that, we can include in v0.10.1. Otherwise, it can go in the next release.

Sorry for the late reply, I had no time at all.

State of okid:

  • I think code is OK, docstring is OK, example is OK!

But:

  • I have to think about the m parameter again. In the paper, the algorithm estimates (m+1) parameters, but it should be consistent with makrov's m parameters.
  • I am not satisfied with my unit tests, maybe someone has suggestions for improvement.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
KybernetikJo added 2 commits February 4, 2025 10:34
@KybernetikJo
Add okid, add okid example
299dd67
@KybernetikJo
Add sphinx doc
5abbfe1
@KybernetikJo
KybernetikJo force-pushed the implement_okid branch from 08c2485 to 5abbfe1 Compare February 4, 2025 10:08
@murrayrm

murrayrm commented Aug 8, 2026

Copy link
Copy Markdown
Member

This draft PR is more than 2 years old. @KybernetikJo: do you want to rebase it so that we can include in the next release (probably sometime in Aug 2026)?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murrayrm murrayrm murrayrm left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

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.