Skip to content

Comments

Implement okid#1031

Draft
KybernetikJo wants to merge 2 commits intopython-control:mainfrom
KybernetikJo:implement_okid
Draft

Implement okid#1031
KybernetikJo wants to merge 2 commits intopython-control:mainfrom
KybernetikJo:implement_okid

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

@murrayrm murrayrm Aug 6, 2024

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

@KybernetikJo KybernetikJo Oct 17, 2024

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

murrayrm commented Aug 13, 2024

@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