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

xferfcn_test test_pole_mimo fails on arm and powerpc #343

Closed
bnavigator opened this issue Nov 9, 2019 · 6 comments
Closed

xferfcn_test test_pole_mimo fails on arm and powerpc #343

bnavigator opened this issue Nov 9, 2019 · 6 comments
Milestone

Comments

@bnavigator
Copy link
Contributor

bnavigator commented Nov 9, 2019

As a continuation of my efforts to provide packages of python-control and slycot for OpenSUSE ( see python-control/Slycot#82 and python-control/Slycot#83), python-control now starts to build (because python-slycot is avaialble now) but fails on test_pole_mimo in xferfcn_test.py:

[  269s] ======================================================================
[  269s] FAIL: Test for correct MIMO poles.
[  269s] ----------------------------------------------------------------------
[  269s] Traceback (most recent call last):
[  269s]   File "/home/abuild/rpmbuild/BUILD/control-0.8.2/control/tests/xferfcn_test.py", line 415, in test_pole_mimo
[  269s]     np.testing.assert_array_almost_equal(p, [-2., -2., -7., -3., -2.])
[  269s]   File "/usr/lib64/python2.7/site-packages/numpy/testing/_private/utils.py", line 1017, in assert_array_almost_equal
[  269s]     precision=decimal)
[  269s]   File "/usr/lib64/python2.7/site-packages/numpy/testing/_private/utils.py", line 754, in assert_array_compare
[  269s]     raise AssertionError(msg)
[  269s] AssertionError: 
[  269s] Arrays are not almost equal to 6 decimals
[  269s] 
[  269s] (shapes (6,), (5,) mismatch)
[  269s]  x: array([-1.999985+0.000000e+00j, -2.000007+1.271532e-05j,
[  269s]        -2.000007-1.271532e-05j, -7.      +0.000000e+00j,
[  269s]        -3.      +0.000000e+00j, -2.      +0.000000e+00j])
[  269s]  y: array([-2., -2., -7., -3., -2.])

_common_den() strikes again (#194, #206). I see it has the optional argument imag_tol, but it is never used within the function except where it is set to 1e-8 by default.

Any ideas how to fix this? Is the resulting machine precision on theses arches really worse than the x86 build, where this tests fine? Or is there maybe a problem with the slycot / lapack / (open)blas / numpy environment on those build machines?

@bnavigator bnavigator changed the title xfercn_test test_pole_mimo fails on arm and powerpc xferfcn_test test_pole_mimo fails on arm and powerpc Nov 9, 2019
@bnavigator
Copy link
Contributor Author

Ignore my last question about slycot. It is not even used in _common_den().
Only numpy.roots() involved in returning complex poles...

@murrayrm
Copy link
Member

murrayrm commented Nov 9, 2019

Assuming that the problem comes down to a difference in precision between architectures, what is the best way to fix this? One possibility would be to introduce a configuration variable for the error tolerance that get used as a default and then this could be set in the test scripts for different architectures (?).

@bnavigator
Copy link
Contributor Author

numpy.finfo().eps as used in _common_den() should be enough to derive the necessary information.

Have look at this one:

    def test_pole_mimo(self):
        """Test for correct MIMO poles."""

        sys = TransferFunction([[[1.], [1.]], [[1.], [1.]]],
                               [[[1., 2.], [1., 3.]], [[1., 4., 4.], [1., 9., 14.]]])
        p = sys.pole()

        try:
            np.testing.assert_array_almost_equal(p, [-2., -2., -7., -3., -2.])
        except AssertionError as e:
            print("EPS: {}".format(np.finfo(float).eps))
            for i in [0, 1]:
                for j in [0, 1]:
                    print("Roots of sys.den[{}][{}]:".format(i, j))
                    print(np.roots(sys.den[i][j]))
            numc, denc, denorderc = sys._common_den()
            print("Common Den:")
            print(denc)
            raise e
[   78s] ...................F..........
[   78s] ======================================================================
[   78s] FAIL: Test for correct MIMO poles.
[   78s] ----------------------------------------------------------------------
[   78s] Traceback (most recent call last):
[   78s]   File "/home/abuild/rpmbuild/BUILD/control-0.8.2/control/tests/xferfcn_test.py", line 426, in test_pole_mimo
[   78s]     raise e
[   78s] AssertionError: 
[   78s] Arrays are not almost equal to 6 decimals
[   78s] 
[   78s] (shapes (6,), (5,) mismatch)
[   78s]  x: array([-1.999985+0.000000e+00j, -2.000007+1.271532e-05j,
[   78s]        -2.000007-1.271532e-05j, -7.      +0.000000e+00j,
[   78s]        -3.      +0.000000e+00j, -2.      +0.000000e+00j])
[   78s]  y: array([-2., -2., -7., -3., -2.])
[   78s] -------------------- >> begin captured stdout << ---------------------
[   78s] EPS: 2.22044604925e-16
[   78s] Roots of sys.den[0][0]:
[   78s] [-2.]
[   78s] Roots of sys.den[0][1]:
[   78s] [-3.]
[   78s] Roots of sys.den[1][0]:
[   78s] [-1.99999999 -2.00000001]
[   78s] Roots of sys.den[1][1]:
[   78s] [-7. -2.]
[   78s] Common Den:
[   78s] [[ 1.  6. 12.  8.]
[   78s]  [ 1. 12. 41. 42.]]
[   78s] 
[   78s] --------------------- >> end captured stdout << ----------------------

Maybe sqrt(eps) (or precision/2) would do the trick?

But I still not fully understand that _common_den algorithm. How would it have to be modified to recognize den[1][0] as a polynomial with a double root?

@murrayrm
Copy link
Member

murrayrm commented Nov 9, 2019

I'm not an expert on this, but my sense is that computing common denominators for MIMO transfer functions is often ill-conditioned. @repagh and @roryyorke are have more expertise here.

@bnavigator
Copy link
Contributor Author

Ok, @repagh, @roryyorke, please have a look at my take in #345. Changing threshold whether we already have the root or not to sqrt(eps) fixes the test failure.

I also changed the logic to actually do something with imag_tol. That last warning in the function never would have triggered because den is only assigned polyfromroots().real further above.

@murrayrm murrayrm added this to the 0.8.3 milestone Dec 29, 2019
@murrayrm
Copy link
Member

Fixed in PR #345

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

No branches or pull requests

2 participants