Merged
Conversation
Merged
bnavigator
reviewed
Dec 26, 2021
Contributor
bnavigator
left a comment
bnavigator
left a comment
There was a problem hiding this comment.
Looks good. A few minor comments:
Comment on lines
+585
to
+590
| elif method == 'slycot': | ||
| return _dare_slycot(A, B, Q, R, S, E, stabilizing) | ||
|
|
||
| else: | ||
| _check_shape("B", B, n, m) | ||
| _check_shape("Q", Q, n, n, square=True, symmetric=True) |
Contributor
There was a problem hiding this comment.
Maybe _dare_slycot() and _dare_scipy() should be on the same level?
Member
Author
There was a problem hiding this comment.
I agree the asymmetry is annoying. I left it this way to avoid changing code that was working. In care everything is in the main function, so at some point we should clean all of these up to be more uniform (either a single function or use _method uniformly).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates the
mateqnmodule to allow the use of SciPy linear algebra functions as a replacement for Slycot functions. Among other things, this allows the use of thelqrcommand without having to install Slycot.I implemented this by changing all of the
mateqnfunctions (lyap,care,dare, etc) to accept atmethodkeyword that can either bescipyorslycot. The default value formethodisslycotif it is installed, otherwisescipy, so there is no change in behavior if you have Slycot.Other changes:
lqrto callcarerather than replicating the code and cleaned up a lot of the code inmateqnto be more Pythonic.slycot_checkfunction so that it stores the result of checking whetherslycotcan be imported rather than trying to import every time the function is called.mateqnfunctions (mateqn._check_shape) that provides more consistent errors (this also required updating some of the unit tests).dareto solve the generalized Sylvester equation usingslycot.sg02adrather than usingslycot.sg02mdto solve the simplified form of the discrete algebraic Riccati equation because there seems to be an error in the way thatstabilizingis handled insg02md(it returns the closed loop eigenvalues in the wrong order). (I'm working on generating a concrete counterexample for posting to Slycot.)