GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — DLQR and DLQE by sawyerbfuller · Pull Request #670 · python-control/python-control

Conversation

sawyerbfuller commented Nov 9, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 commented Nov 9, 2021

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sawyerbfuller commented Nov 9, 2021

Copy link

Copy Markdown

Contributor Author

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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator reviewed

Nov 9, 2021

View reviewed changes

Comment thread

control/tests/statefbk_test.py Outdated

Comment on lines +413 to +415

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

bnavigator Nov 9, 2021

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sawyerbfuller Nov 9, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator reviewed

Nov 9, 2021

View reviewed changes

Comment thread

control/mateqn.py Outdated

Comment on lines +500 to +502

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

bnavigator Nov 9, 2021

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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 for stabilizing=True. I guess the SLICOT routine should be more efficient than calling 3 solvers for X, G, L.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator Nov 9, 2021

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sawyerbfuller Nov 9, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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 a method keyword as suggested in #255.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator reacted with thumbs up emoji

All reactions

👍 1 reaction

bnavigator Nov 9, 2021

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sawyerbfuller Nov 11, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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’s dare. I’ll see if I can find the bug. Ok if we leave adding a method keyword to a different PR?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator Nov 11, 2021

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator reviewed

Nov 9, 2021

View reviewed changes

Comment thread

control/mateqn.py Outdated

Comment on lines +233 to +234

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

bnavigator Nov 9, 2021

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sawyerbfuller Nov 9, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator reacted with thumbs up emoji

All reactions

👍 1 reaction

coveralls commented Nov 9, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

sawyerbfuller commented Nov 23, 2021

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm mentioned this pull request

Nov 26, 2021

Improved lqe calling functionality #673

Merged

murrayrm commented Nov 26, 2021

Copy link

Copy Markdown

Member

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 (?).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sawyerbfuller commented Nov 26, 2021

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator mentioned this pull request

Dec 2, 2021

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

Merged

murrayrm commented Dec 24, 2021

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Dec 26, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

sawyerbfuller commented Dec 27, 2021

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix lqe docstring per @sawyerbfuller

0d4ff4c

murrayrm commented Dec 27, 2021

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm added this to the 0.9.1 milestone

Dec 30, 2021

murrayrm merged commit a33bcc5 into python-control:master

Dec 31, 2021

Reviewers

bnavigator

bnavigator left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.1

Development

Successfully merging this pull request may close these issues.

Discrete Time LQR Option

Uh oh!

There was an error while loading. Please reload this page.

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

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sawyerbfuller commented Nov 9, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

coveralls commented Nov 9, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

murrayrm commented Dec 26, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.