Dgebal application order #122
Conversation
slycot/tests/test_tb05ad.py
Outdated
| @@ -11,6 +11,7 @@ | |||
| from numpy.testing import assert_almost_equal | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
E303, too many blank lines
There was a problem hiding this comment.
Should be fixed in the commit.
slycot/tests/test_tb05ad.py
Outdated
| def test_tb05ad_ag(self): | ||
| """ | ||
| Test that tb05ad with job 'AG' computes the correct | ||
| frequency response. | ||
| """ | ||
| for key in CASES: | ||
| sys = CASES[key] | ||
| self.check_tb05ad_AG_NG(sys, 10*1j, 'AG') | ||
|
|
||
| def test_tb05ad_ag_fixed_bug_no11(self): | ||
| """ Test tb05ad and job 'AG' (i.e., balancing enabled). | ||
|
|
||
| Failed on certain A matrices before, issue #11. | ||
| """ | ||
| self.check_tb05ad_AG_NG(CASES['fail1'], 10*1j, 'AG') |
There was a problem hiding this comment.
This just runs CASES['fail1'] twice. Probably the name 'fail1' is not appropriate anymore.
| def test_tb05ad_ag(self): | |
| """ | |
| Test that tb05ad with job 'AG' computes the correct | |
| frequency response. | |
| """ | |
| for key in CASES: | |
| sys = CASES[key] | |
| self.check_tb05ad_AG_NG(sys, 10*1j, 'AG') | |
| def test_tb05ad_ag_fixed_bug_no11(self): | |
| """ Test tb05ad and job 'AG' (i.e., balancing enabled). | |
| Failed on certain A matrices before, issue #11. | |
| """ | |
| self.check_tb05ad_AG_NG(CASES['fail1'], 10*1j, 'AG') | |
| def test_tb05ad_ag(self): | |
| """ | |
| Test that tb05ad with job 'AG' computes the correct | |
| frequency response with balancing A first. | |
| Failed on certain A matrices before, issue #11. | |
| """ | |
| for key in CASES: | |
| sys = CASES[key] | |
| self.check_tb05ad_AG_NG(sys, 10*1j, 'AG') |
There was a problem hiding this comment.
renamed, and removed the repeat
slycot/tests/test_tb05ad.py
Outdated
| def test_tb05ad_balance(self): | ||
| """Specifically check for the balancing output. | ||
|
|
||
| Based on https://rdrr.io/rforge/expm/man/balance.html |
There was a problem hiding this comment.
According to that link, this case has SCALE=[1 1 1 3], ILO=2, IHI=3 and thus only swaps 4 with 3 and 1 with 1. Does not really check the correct order of transformations done, does it?
To check the newly corrected transformation in TB05AD, Bt, Ct need to be checked for correct order as well.
There was a problem hiding this comment.
I created a new test, based on a random matrix, tweaked until that one showed permutation
There was a problem hiding this comment.
Could this be pinned to a fixed found matrix? Having random data in tests that could fail at times and does not at other times will make debugging hard.
There was a problem hiding this comment.
we're still covered by the random seed at the start of the file
There was a problem hiding this comment.
Ah okay, I didn't see that.
slycot/tests/test_tb05ad.py
Outdated
| # and verify that eigenvalues of both A matrices are close | ||
| assert_almost_equal(np.dot(np.dot(Ct, At), Bt), | ||
| np.dot(np.dot(C, A), B)) | ||
| assert_almost_equal(eig(At)[0], eig(A)[0]) |
There was a problem hiding this comment.
Is the order and scaling of eigenvalues guaranteed? Could run into assertion failures on some build systems. Or do scipy.linalg.eig() and MB05AD call the same LAPACK code and thus always yield the same results?
There was a problem hiding this comment.
I'll put in some sorting.
There was a problem hiding this comment.
What about scaling, including different signs? I had to deal with that in test_mb05md() (#79)
There was a problem hiding this comment.
Now you're confusing me. Do you mean scaling of the eigenvalues? But then the frequency should also be scaled to calculate the response, and I don't see anything on that in the TB05AD.f code. Eigenvectors may be differently scaled, but I did not do a test on that.
Or do you mean the scaling in the matrix_balance / dgebal step. I now added some code for that, got to the same testing case as before, on the 10th matrix tried.
There was a problem hiding this comment.
Okay strike that. Brainfart by me. Mixed up eigenvalue properties with eigenvectors.
Should address issue #11