DLQR and DLQE

[sawyerbfuller]

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)

[bnavigator]

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

[sawyerbfuller]

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]

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)

?

[sawyerbfuller]

thanks for the suggestion & code sketch. done

[bnavigator]

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.

[bnavigator]

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

[sawyerbfuller]

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

[bnavigator]

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

[sawyerbfuller]

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?

[bnavigator]

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]

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

[sawyerbfuller]

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]

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

[sawyerbfuller]

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]

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]

@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]

@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.

[murrayrm]

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]

@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]

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.