Skip to content

First commit of balred2() added to modelsimp.py. #78

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

Closed
wants to merge 4 commits into from
Closed

First commit of balred2() added to modelsimp.py. #78

wants to merge 4 commits into from

Conversation

mdclemen
Copy link
Contributor

This pull request contains a function, balred2(), which will do a balanced, truncated realization of an unstable LTI model.

@cwrowley
Copy link
Contributor

cwrowley commented Mar 2, 2016

Looks promising. I haven't looked at this closely, but I have a couple of remarks at first glance:

  • Looks like the Travis tests are failing
  • Before this is merged in, it will be important to add unit tests for the new routine
  • I personally think it's a bad idea to have two routines balred() and balred2() with similar functionality and different interfaces. I'd prefer to see a single routine, for instance with an optional argument to specify which algorithm to use.

@roryyorke
Copy link
Contributor

The test failure is the same thing as reported in #70; exactly the same gain margins were found by @whiplash01 as Travis produced for Python 2.7 and 3.4 here.

I can't reproduce this on my machine (Ubuntu 14.04, Python 3.4.3) with "own-built" Slycot and python-control. I have struggled to reproduce the Travis environment with my relatively slow network connection; Conda seems to want to download an awful lot of things, and then change it's mind about Python versions, etc.

Anyway, it looks like there's a subtle bug lurking in the gain margin code.

@mdclemen
Copy link
Contributor Author

mdclemen commented Mar 8, 2016

I have modified balred() to now just check for whether or not unstable eigenvalues exist, and if so, perform the necessary steps to do the balancing and truncating.

@mdclemen
Copy link
Contributor Author

mdclemen commented Mar 8, 2016

My first two commits were failing on testBalred(), when orders was passed as a vector(list). I've fixed this error and now the balred() function will return a vector(list) of reduced order models if orders is passed as a vector. I don't know what is going on with the testCombi01().

@murrayrm
Copy link
Member

Travis build is failing due to older version of conda (fixed in f334b1d). This will be corrected when code is merged into master.

@murrayrm
Copy link
Member

@mdclemen: Need unit tests before this can be merged into master. Also, if you can grab the update to .travis.yml from f334b1d, you can get Travis CI build working.

@mdclemen
Copy link
Contributor Author

Could you please explain what unit tests are or direct me to a reference.

@murrayrm
Copy link
Member

murrayrm commented Jun 1, 2016

A good introduction is here:

https://en.wikipedia.org/wiki/Unit_testing

If you look in control/tests, you'll see a large collection of unit tests for different functions. If you copy one of those into something called balred_test.py and then put in some valid unit tests, we can use that to make sure that everything is working under all of the different platforms. margin_test.py is a reasonably good sample.

@cwrowley
Copy link
Contributor

cwrowley commented Jun 1, 2016

Since the modified routine is in modelsimp.py, my suggestion would be to put the new tests in tests/modelsimp_test.py, following the NumPy/SciPy Testing Guidelines.

Also, if you rebase your branch to the current master branch, then the tests will be run automatically by Travis CI when you push more commits to this pull request. The current version in this PR has a bug in setting things up on Travis CI, which leads to the error showing up here.

(By the way, @murrayrm, it might be good at some point to organize the existing tests better, to follow the NumPy/SciPy guidelines, for instance so that tests for module foo.py are in test_foo.py. Many are already like this, but some modules do not have corresponding test modules.)

…unstable systems along with 'matchdc' gain option. Also added option to gram() to return cholesky factored gramian.
@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage increased (+0.7%) to 74.766% when pulling b72da8a on mdclemen:mdc-dev into 49045ca on python-control:master.

@mdclemen mdclemen closed this Aug 18, 2016
@slivingston
Copy link
Member

@mdclemen, will you consider opening a new pull request or reworking commits on this one (and re-opening it)? I can work with you to make unit tests.

@mdclemen
Copy link
Contributor Author

@slivingston, I have made updates to my fork of python-control and it requires the updates I made to slycot. I will wait to get my pull for python-control/slycot merged first.

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.

6 participants