Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Dec 28, 2020

Conversation

murrayrm
Copy link
Member

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

These files will need to be updated again in the 0.9.0 release so that the default for the transpose keyword changes from True to False.

@coveralls
Copy link

coveralls commented Dec 24, 2020

Coverage Status

Coverage decreased (-0.03%) to 86.328% when pulling 438c178 on murrayrm:markov-inconsistency into 120a926 on python-control:master.

@bnavigator
Copy link
Contributor

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))

@murrayrm
Copy link
Member Author

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 original markov_test.py (to make sure tests are backward compatible) and moved all of the "enhanced" testing into markov_test_array.py.
  • Added a new unit test in markov_test_array.py to make sure the calculation is correct.

@murrayrm murrayrm linked an issue Dec 26, 2020 that may be closed by this pull request
@murrayrm murrayrm merged commit bd354c9 into python-control:master Dec 28, 2020
@murrayrm murrayrm deleted the markov-inconsistency branch December 28, 2020 22:43
@murrayrm murrayrm added this to the 0.9.0 milestone Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

control.matlab.markov does not work
3 participants