Coverage decreased (-2.8%) to 80.965% when pulling712f78a on bnavigator:fix-armfail intoa4b4c43 on python-control:master.

Coverage decreased (-3.3%) to 80.447% when pullingd09472c on bnavigator:fix-armfail intoa4b4c43 on python-control:master.

I don't understand why the unit test coverage is going down so much as a result of these changes.

Everything looks OK; will merge in a few days unless anyone has objections.

I don't understand why the unit test coverage is going down so much as a result of these changes.

Everything looks OK; will merge in a few days unless anyone has objections.

Coverage : I don't understand it fully either, after comparing the results of the last master build vs. this pull request.

Coveralls might be giving down points because the newly addedwarn statement is untested.

Coverage goes down because the new warning line about the nontrivial imaginary part is not reached by the test.

The more I think about it, I don't like the way the function discards the imaginary part. There should still be a warning that the result has imaginary coefficients, because the SLICOT routines asking for a common den cannot handle it, but they should not be discarded.

Also I would like to get rid of thetol_imag parameter. It does not do what the documentation says it does. It is not used within python-control and changing the parameters of an internal function should not break any external programs.

I would like to write some more extensive unit tests to check the proper calculation of common denominators and also the warnings before this is to be merged.

Coverage goes down because the new warning line about the nontrivial imaginary part is not reached by the test.

This would explain coverage going down a small amount, but 3% (of the entire code base)? Seems off.

I agree on the desire to do something sensible withtol_imag. Will hold on merging this PR for now.

I think it is because some of the configured travis jobs without slycot skip most unit tests. (#349)

I made some modifications and added an unit test. Please review.

imag_tol is now used as originally intended and documented. The algorithm now checks for equality to known poles based onsqrt(eps), because they are theroots of the polynomial coefficients. Thus, the precision error from the coefficientseps propagates to the larger valuesqrt(eps).

Confirmed that unit tests are working with PR#353 => ready to merge.