NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

DLQR and DLQE#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

murrayrm merged 18 commits intopython-control:masterfromsawyerbfuller:dlqr2

Dec 31, 2021

Merged

DLQR and DLQE#670

murrayrm merged 18 commits intopython-control:masterfromsawyerbfuller:dlqr2

Dec 31, 2021

Conversation

Copy link

Contributor

sawyerbfuller commentedNov 9, 2021•
edited
Loading

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 ofcare anddare that have non-slycot options available.

care anddare now use scipysolve_continuous_are andsolve_discrete_are if possible instead of slycot (only case they don't is whenstabilizing=False)

sawyerbfullerand others added2 commits

May 19, 2020 11:44

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

e2d8f9d

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

0854ee4

…trol

sawyerbfuller linked an issue

Nov 9, 2021

that may beclosed by this pull request

Discrete Time LQR Option#359

Closed

Copy link

Contributor

bnavigator commentedNov 9, 2021

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

Copy link

ContributorAuthor

sawyerbfuller commentedNov 9, 2021

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 reviewed

Nov 9, 2021

View reviewed changes

control/tests/statefbk_test.py Outdated

Comment on lines 413 to 415

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

Copy link

Contributor

bnavigatorNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

How about instead of duplicating code lines, you parameterizetest_care andtest_dare with something like

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

Copy link

ContributorAuthor

sawyerbfullerNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

thanks for the suggestion & code sketch. done

bnavigator reviewed

Nov 9, 2021

View reviewed changes

control/mateqn.py Outdated

Comment on lines 500 to 502

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

Copy link

Contributor

bnavigatorNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

bnavigatorNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Found7cf9630 and#8. Has this ever been investigated thoroughly?

Copy link

ContributorAuthor

sawyerbfullerNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

bnavigatorNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

sawyerbfullerNov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

bnavigatorNov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 reviewed

Nov 9, 2021

View reviewed changes

control/mateqn.py Outdated

Comment on lines 233 to 234

		raiseControlArgument("Q must be a symmetric matrix.")
		raiseControlDimension("Q must be a symmetric matrix.")

Copy link

Contributor

bnavigatorNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

sawyerbfullerNov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

coveralls commentedNov 9, 2021•
edited
Loading

Coverage increased (+0.1%) to 93.655% when pulling0d4ff4c on sawyerbfuller:dlqr2 into8356044 on python-control:master.

bnavigator mentioned this pull request

Nov 9, 2021

Add Apple Silicon buildsconda-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

Copy link

ContributorAuthor

sawyerbfuller commentedNov 23, 2021

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 mentioned this pull request

Nov 26, 2021

Improved lqe calling functionality#673

Merged

Copy link

Member

murrayrm commentedNov 26, 2021

Some (minor) overlapping changes in#673, which allows thelqe function to be called with aStateSpace object as the first argument. We might consider adding some of the functionality for#673 into this PR, for consistency. We could also updatelqr andlqe so that if they are passed a discrete timeLTI system as their first argument, they calldlqr anddlqe directly (?).

Copy link

ContributorAuthor

sawyerbfuller commentedNov 26, 2021

@murrayrm I like the idea of "overloading"lqe andlqr 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.

bnavigator mentioned this pull request

Dec 2, 2021

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

Merged

Copy link

Member

murrayrm commentedDec 24, 2021

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

murrayrm added the needs rebaseThe PR needs a rebase to current master branch label

Dec 26, 2021

murrayrmand others added10 commits

December 26, 2021 12:01

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

4c79cb2

Merge branch 'master' ofhttps://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 thedlqr2 branch from82d328a to3e99742Compare

December 26, 2021 21:25

murrayrm removed the needs rebaseThe PR needs a rebase to current master branch label

Dec 26, 2021

Copy link

Member

murrayrm commentedDec 26, 2021•
edited
Loading

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

Add timebase checks tolqr() to call thelqr ordlqr, as appropriate
Regularize the use of_method incare anddare
Remove redundant argument processing indare and_dare_slycot (since the latter is an internal function)

murrayrm added4 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

Copy link

ContributorAuthor

sawyerbfuller commentedDec 27, 2021

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

One remark in regards to a change on line 280 instatefbk.py that changedB toG. I think it should remainB. In the general case there should be a distinction between theB (input) andG (disturbance input) matrices , and it is nice to have aG in case you have disturbances that only act on a subset of states (eg force but not velocity disturbances).

The question is, what iflqe is passed a system? Then I think it is ok to interpret the system’sB matrix asG, 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.

Fix lqe docstring per@sawyerbfuller

0d4ff4c

Copy link

Member

murrayrm commentedDec 27, 2021

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.

murrayrm added this to the0.9.1 milestone

Dec 30, 2021

murrayrm merged commita33bcc5 intopython-control:master

Dec 31, 2021

Labels

None yet

4 participants

Movatterモバイル変換

DLQR and DLQE#670

DLQR and DLQE#670

Uh oh!

Conversation

sawyerbfuller commentedNov 9, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

bnavigator commentedNov 9, 2021

Uh oh!

sawyerbfuller commentedNov 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 commentedNov 9, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

sawyerbfuller commentedNov 23, 2021

Uh oh!

murrayrm commentedNov 26, 2021

Uh oh!

sawyerbfuller commentedNov 26, 2021

Uh oh!

murrayrm commentedDec 24, 2021

Uh oh!

murrayrm commentedDec 26, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

sawyerbfuller commentedDec 27, 2021

Uh oh!

murrayrm commentedDec 27, 2021

Uh oh!

Uh oh!

sawyerbfuller commentedNov 9, 2021•
edited
Loading

coveralls commentedNov 9, 2021•
edited
Loading

murrayrm commentedDec 26, 2021•
edited
Loading