Skip to content

Implement okid #1031

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Implement okid #1031

wants to merge 2 commits into from

Conversation

KybernetikJo
Copy link
Contributor

@KybernetikJo KybernetikJo commented Jul 15, 2024 •
edited
Loading

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)

@coveralls
Copy link

coveralls commented Jul 15, 2024 •
edited
Loading

Coverage Status

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

@KybernetikJo KybernetikJo force-pushed the implement_okid branch 2 times, most recently from 860acb7 to 67d6ef0 Compare July 16, 2024 09:47
@murrayrm
Copy link
Member

murrayrm commented Aug 6, 2024

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

murrayrm
murrayrm reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@murrayrm 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 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?

control/modelsimp.py
-------
H : ndarray
First m Markov parameters, [D CB CAB ...].

Copy link
Member

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?

Copy link
Contributor Author

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.

@murrayrm
Copy link
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.

@KybernetikJo
Copy link
Contributor Author

KybernetikJo commented Oct 19, 2024 •
edited
Loading

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KybernetikJo @coveralls @murrayrm