NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

updated markov() to add tranpose keyword + default warning#478

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

murrayrm merged 3 commits intopython-control:masterfrommurrayrm:markov-inconsistency

Dec 28, 2020

Merged

updated markov() to add tranpose keyword + default warning#478

murrayrm merged 3 commits intopython-control:masterfrommurrayrm:markov-inconsistency

Dec 28, 2020

Conversation

Copy link

Member

murrayrm commentedDec 24, 2020

This PR partially addresses issue#395 by adding atranspose keyword to themarkov function that allows transposed inputs. In the current release, themarkov function is not consistent with the normal time-series convention (see discussion in#395) so a warning message is generated iftranspose is not set explicitly toTrue.

These files will need to be updated again in the 0.9.0 release so that the default for thetranspose keyword changes fromTrue toFalse.

updated markov() to add tranpose keyword + default warning; tests, PEP8

7ed68ec

Copy link

coveralls commentedDec 24, 2020•
edited
Loading

Coverage decreased (-0.03%) to 86.328% when pulling438c178 on murrayrm:markov-inconsistency into120a926 on python-control:master.

Copy link

Contributor

bnavigator commentedDec 24, 2020

This PR itself LGTM. But are we sure that the algorithm actually works for real data? The data used in the test is worthless to determine any markov parameters. (#395 (comment))

updated markov calculation + new unit tests

bb15b85

Copy link

MemberAuthor

murrayrm commentedDec 26, 2020

Good point on whether the calculation was right. Looks like it wasn't...

In this latest push, I have:

Updated the markov() algorithm to compute the correct coefficients (the old code is there for comparison, along with a new algorithm + reference).
Reverted to the originalmarkov_test.py (to make sure tests are backward compatible) and moved all of the "enhanced" testing intomarkov_test_array.py.
Added a new unit test inmarkov_test_array.py to make sure the calculation is correct.