Refactored matrix inversions (see #85 and #91) #101
Conversation
|
The original "yottalab" project doesn't exixt anymore. It has been
and
All the other functions of the original yottalab project have migrated Best regards Roberto BTW: For the Swiss Control Association I've prepared a little On 07/03/2016 04:36 PM, Mikhail Pak wrote:
|
|
1fe639c: After some consideration I've also replaced the rank check with both Case in point: import numpy as np
A = np.matrix("1.0e-3, 0.0; 0.0, 1.0e-3")
print(np.linalg.det(A))
print(np.linalg.matrix_rank(A))This change does affect unit tests: Fewer systems are skipped in |
|
bf6c446: Added singularity check before matrix inversion for DC matching / residualization. I took the liberty to change the unit test slightly, since it does not really matter anyway (see the docstring, it just calls MATLAB-like methods and does not check the results). Instead of eliminating the 2nd and the 3rd state variable, it now eliminates the 1st and the 2nd. 2c25b73: Adds a unit test to check residualization of unstable systems (should throw a 32323d4: Made code more pythonic, in this case list comprehensions instead of for-loops. Also call Edit: Spelling |
|
See https://travis-ci.org/mp4096/python-control/builds/144536541 for CI results with the correct configuration from #102. |
|
I will review this tonight. |
* The performance remains the same. * The source code is more readable.
Also improved the error message for the singularity check.
* `numpy.linalg.solve` instead of matrix inversion. * Better controllability matrix rank check.
Also made unity elements of the A, B matrices float -- just in case.
And a corresponding unit test
* Changed the unit test in matlab_test.py so that no error is thrown
Mainly list comprehensions instead of for-loops
Faster than converting to a pure Python list.
1 similar comment
|
For posterity, the "old TODO" that is referenced in the opening post of this pull request is in control/statesp.py at line 127 as of commit c498d3d (current tip of |
|
The change in your first commit (4d70d00) refactors some code to use I compared the two alternatives using %timeit of Jupyter for matrices that would correspond to LTI systems with 10 state dimensions, 2 inputs, 2 outputs (so, the A matrix has shape (10, 10), B matrix shape (10,2), etc.), and with 100 state dimensions, 20 inputs, 20 outputs, all using matrices from numpy.random.random((r,c)). The difference in timing performance does not appear significant. However, the case of |
#101 Changes are from branch `master` of https://github.com/mp4096/python-control.git There was merge conflict in how a for-loop was refactored into `map` (here) vs. list comprehension (from PR #110). I compared the two alternatives using %timeit of Jupyter for matrices that would correspond to LTI systems with 10 state dimensions, 2 inputs, 2 outputs (so, the A matrix has shape (10, 10), B matrix has shape (10,2), etc.), and with 100 state dimensions, 20 inputs, 20 outputs, all using matrices from numpy.random.random((r,c)). The difference in timing performance does not appear significant. However, the case of `map` was slightly faster (approximately 500 to 900 ns less in duration), so I decided to use that one to resolve the merge conflict.
|
@mp4096 Thanks for these improvements of numerical reliability. As future work, I think that it would be interesting to add more test cases that would fail to be detected if |
Continuing work on the issue raised in #85 and partially done in #91:
.Is andinvs withnumpy.linalg.solveornumpy.linalg.lstsq. The remaining inversions which I didn't touch are inyottalab.py. Is it still used?A\B,A\Cby solvingA\[B, C]and taking views for the corresponding results. This avoids redundant cubic LU decompositions.compatability->compatabilityI commented out the matrix singularity check inSee comment for bf6c446modelsimp.pysince it correctly raises an error duringtestModred()inmatlab_test.py. MatrixA22is indeed numerically singular in this case (second column is something like[[0.0, 3.0e-16]]). Should we change the test case?