DLQR and DLQE by sawyerbfuller · Pull Request #670 · python-control/python-control

sawyerbfuller · 2021-11-09T05:19:41Z

update 2021.1.09
Slycot not currently working with my new M1 mac so I made it so lqr, lqe, dlqr, and dlqe now have a fallback to scipy-only. They use an updated version of care and dare that have non-slycot options available.

care and dare now use scipy solve_continuous_are and solve_discrete_are if possible instead of slycot (only case they don't is when stabilizing=False)

…trol

bnavigator · 2021-11-09T22:00:02Z

Any indication, why you can't have Slycot on your M1? No conda packages?

sawyerbfuller · 2021-11-09T22:08:57Z

Any indication, why you can't have Slycot on your M1? No conda packages?

Not sure. conda has been spending a lot of time "solving" the environment and then not succeeding (I don't remember the error message) when I try to install slycot with conda-forge. Will try again

bnavigator · 2021-11-09T22:11:23Z

control/tests/statefbk_test.py

+    @slycotonly
+    def test_care_antistabilizing(self, matarrayin):
+        """Test anti-stabilizing feedbacks, continuous"""


How about instead of duplicating code lines, you parameterize test_care and test_dare with something like

@pytest.mark.parametrize( "stabilizing", [True, pytest.param(False, marks=slycotonly), ] def test_care(self, matarrayin, stabilizing): ... X, L, G = care(A, B, Q, R, S, E, stabilizing=stabilizing) sgn = {True: -1, False: 1}[stabilizing] assert np.all((sgn * np.real(L)) > 0)

?

thanks for the suggestion & code sketch. done

bnavigator · 2021-11-09T22:20:59Z

control/mateqn.py

+    if not stabilizing:
+        return care_slycot(A, B, Q, R, S, E, stabilizing)
+    else:


This always favors the SciPy version over Slycot for stabilizing=True. I guess the SLICOT routine should be more efficient than calling 3 solvers for X, G, L.

Found 7cf9630 and #8. Has this ever been investigated thoroughly?

Might be nicer to implement a method keyword as suggested in #255.

I guess the significant number of uncovered lines as reported by coveralls comes from this change.

One of my grad students actually encountered that bug with slycot’s dare. I’ll see if I can find the bug. Ok if we leave adding a method keyword to a different PR?

I'm would be okay with deferring the method keyword to a later PR.

But I don't like leaving code in the repo which is essentially untested or disabled.

bnavigator · 2021-11-09T22:37:37Z

control/mateqn.py

-            raise ControlArgument("Q must be a symmetric matrix.")
+            raise ControlDimension("Q must be a symmetric matrix.")


That's not a dimension error. ControlDimension would have been raised above.

True. I was trying to make it so both slycot and scipy versions raised the same kind of error (ValueError) rather than different types (ControlArgument is a TypeError), so that we could run the same test on both. win-win solution: I'll just change that to a ValueError because I think it can be argued that being non-symmetric is more of a value error than a type error.

coveralls · 2021-11-09T22:45:00Z

Coverage increased (+0.1%) to 93.655% when pulling 0d4ff4c on sawyerbfuller:dlqr2 into 8356044 on python-control:master.

sawyerbfuller · 2021-11-23T22:42:17Z

Marked this as ready for a review to merge. Still working on getting slycot working on my mac (reinstall anaconda didn't do it) so I think a PR that adds a method keyword will have to wait until I get that resolved.

murrayrm · 2021-11-26T18:05:25Z

Some (minor) overlapping changes in #673, which allows the lqe function to be called with a StateSpace object as the first argument. We might consider adding some of the functionality for #673 into this PR, for consistency. We could also update lqr and lqe so that if they are passed a discrete time LTI system as their first argument, they call dlqr and dlqe directly (?).

sawyerbfuller · 2021-11-26T22:56:22Z

@murrayrm I like the idea of "overloading" lqe and lqr to work as the corresponding discrete-time versions when passed a discrete-time SS system. Possible rationale: they're both still "linear quadratic regulators/estimators".

Happy to rebase this one on top of #673, or vice versa, but it may be a few days before I have time to do anything on it.

murrayrm · 2021-12-24T15:17:48Z

@sawyerbfuller FYI, if you rebase this, you might want to wait until #683 is merged since that has a lot of relevant changes in both mateqn and statefbk.

…trol

…ug in non-slycot care. lqr, lqe, dlqr, dlqe now get tested without slycot.

…when arrays are not of the correct dimension

murrayrm · 2021-12-26T21:30:20Z

Rebased on latest master. A couple of other things we could do before merging:

Add timebase checks to lqr() to call the lqr or dlqr, as appropriate
Regularize the use of _method in care and dare
Remove redundant argument processing in dare and _dare_slycot (since the latter is an internal function)

sawyerbfuller · 2021-12-27T06:37:22Z

@murrayrm thanks for moving this one toward the finish line.

One remark in regards to a change on line 280 in statefbk.py that changed B to G. I think it should remain B. In the general case there should be a distinction between the B (input) and G (disturbance input) matrices , and it is nice to have a G in case you have disturbances that only act on a subset of states (eg force but not velocity disturbances).

The question is, what if lqe is passed a system? Then I think it is ok to interpret the system’s B matrix as G, as currently implemented. But I think the place to state that is down at the end of the doc string where the option to pass a system is described.

murrayrm · 2021-12-27T15:14:43Z

One remark in regards to a change on line 280 in statefbk.py that changed B to G. I think it should remain B. In the general case there should be a distinction between the B (input) and G (disturbance input) matrices , and it is nice to have a G in case you have disturbances that only act on a subset of states (eg force but not velocity disturbances).

Oops. Reverted.

sawyerbfuller and others added 2 commits May 19, 2020 11:44

added link to lqe (linear quadratic estimator) function in docs

e2d8f9d

Merge branch 'master' of https://github.com/python-control/python-con…

0854ee4

…trol

sawyerbfuller linked an issue Nov 9, 2021 that may be closed by this pull request

Discrete Time LQR Option #359

Closed

bnavigator reviewed Nov 9, 2021

View reviewed changes

bnavigator mentioned this pull request Nov 9, 2021

Add Apple Silicon builds conda-forge/slycot-feedstock#34

Closed

sawyerbfuller marked this pull request as draft November 10, 2021 17:41

sawyerbfuller marked this pull request as ready for review November 23, 2021 22:40

sawyerbfuller changed the title ~~[WIP] DLQR and DLQE~~ DLQR and DLQE Nov 23, 2021

murrayrm mentioned this pull request Nov 26, 2021

Improved lqe calling functionality #673

Merged

bnavigator mentioned this pull request Dec 2, 2021

Remove duplicate Slycot error handling, require Slycot >=0.4 #678

Merged

murrayrm added the needs rebase The PR needs a rebase to current master branch label Dec 26, 2021

murrayrm and others added 10 commits December 26, 2021 12:01

Merge branch 'master' of https://github.com/sawyerbfuller/python-control

4c79cb2

Merge branch 'master' of https://github.com/python-control/python-con…

7076fac

…trol

enable non-slycot lqr

b382f2d

initial addition of dlqe and dlqr

04cfe65

dare and care now use scipy routines if slycot not available. fixed b…

0fa431c

…ug in non-slycot care. lqr, lqe, dlqr, dlqe now get tested without slycot.

enabled some non-slycot testing of dare and care functions in mateqn.py

0af8f83

change exceptions to controlDimension instead of controlArgument for …

798ccd5

…when arrays are not of the correct dimension

correct the expected error type in mateqn_test.py

88960af

one more error type correction

90ad3a6

shortened testing code as suggested by @bnavigator

e26f765

enable non-slycot testing + minor cleanup after rebase

3e99742

murrayrm force-pushed the dlqr2 branch from 82d328a to 3e99742 Compare December 26, 2021 21:25

murrayrm removed the needs rebase The PR needs a rebase to current master branch label Dec 26, 2021

murrayrm added 4 commits December 26, 2021 15:38

add lqr/lqe overload for discrete time systems

20b0312

remove redundant argument process in care/dare + fix up error strings

cf3c9b2

remove _dare_slycot and make slycot/scipy structure moer uniform

f1f5375

require StateSpace for lqr/lqe first arg + process uniformly

fdd399f

Fix lqe docstring per @sawyerbfuller

0d4ff4c

murrayrm added this to the 0.9.1 milestone Dec 30, 2021

murrayrm merged commit a33bcc5 into python-control:master Dec 31, 2021

		raise ControlArgument("Q must be a symmetric matrix.")
		raise ControlDimension("Q must be a symmetric matrix.")

Conversation

sawyerbfuller commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnavigator commented Nov 9, 2021

Uh oh!

sawyerbfuller commented Nov 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sawyerbfuller commented Nov 23, 2021

Uh oh!

murrayrm commented Nov 26, 2021

Uh oh!

sawyerbfuller commented Nov 26, 2021

Uh oh!

murrayrm commented Dec 24, 2021

Uh oh!

murrayrm commented Dec 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sawyerbfuller commented Dec 27, 2021

Uh oh!

murrayrm commented Dec 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sawyerbfuller commented Nov 9, 2021 •

edited

Loading

coveralls commented Nov 9, 2021 •

edited

Loading

murrayrm commented Dec 26, 2021 •

edited

Loading