GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — LQR using SciPy by murrayrm · Pull Request #683 · python-control/python-control

Conversation

@murrayrm

@murrayrm murrayrm commented Dec 24, 2021

Copy link
Copy Markdown
Member

This PR updates the mateqn module to allow the use of SciPy linear algebra functions as a replacement for Slycot functions. Among other things, this allows the use of the lqr command without having to install Slycot.

I implemented this by changing all of the mateqn functions (lyap, care, dare, etc) to accept at method keyword that can either be scipy or slycot. The default value for method is slycot if it is installed, otherwise scipy, so there is no change in behavior if you have Slycot.

Other changes:

  • Updated lqr to call care rather than replicating the code and cleaned up a lot of the code in mateqn to be more Pythonic.
  • Modified the slycot_check function so that it stores the result of checking whether slycot can be imported rather than trying to import every time the function is called.
  • Implemented a function for checking size and properties of matrices passed to mateqn functions (mateqn._check_shape) that provides more consistent errors (this also required updating some of the unit tests).
  • Modified dare to solve the generalized Sylvester equation using slycot.sg02ad rather than using slycot.sg02md to solve the simplified form of the discrete algebraic Riccati equation because there seems to be an error in the way that stabilizing is handled in sg02md (it returns the closed loop eigenvalues in the wrong order). (I'm working on generating a concrete counterexample for posting to Slycot.)

Sorry, something went wrong.

Uh oh!

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

All reactions
@murrayrm murrayrm mentioned this pull request Dec 24, 2021
DLQR and DLQE #670
Merged
@coveralls

coveralls commented Dec 24, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.7%) to 93.433% when pulling 801f282 on murrayrm:lqr_scipy into 4f681ab on python-control:master.

All reactions

Sorry, something went wrong.

Uh oh!

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

murrayrm added 6 commits December 26, 2021 06:30
@murrayrm
add method='scipy' to lqr()
91a0455
@murrayrm
cache status of slycot in slycot_check()
88da729
@murrayrm
simplify mateqn argument processing/checking
69e9605
@murrayrm
add method='scipy' to mateqn functions
4baea13
@murrayrm
use care() for lqr()
b284c47
@murrayrm
fix up rebase issues
53f7808
@murrayrm
murrayrm force-pushed the lqr_scipy branch from a444efb to 53f7808 Compare December 26, 2021 14:49
bnavigator
bnavigator reviewed Dec 26, 2021
View reviewed changes

@bnavigator bnavigator left a comment

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.

Looks good. A few minor comments:

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/tests/statefbk_test.py Resolved

Uh oh!

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

Comment thread control/mateqn.py Resolved

Uh oh!

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

Comment thread control/mateqn.py
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)

@bnavigator bnavigator Dec 26, 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.

Maybe _dare_slycot() and _dare_scipy() should be on the same level?

Sorry, something went wrong.

Uh oh!

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

All reactions

@murrayrm murrayrm Dec 26, 2021

Copy link
Copy Markdown
Member 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.

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

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
@murrayrm
address @bnavigator comments
801f282
@bnavigator
bnavigator merged commit a05351b into python-control:master Dec 26, 2021
@bnavigator bnavigator mentioned this pull request Dec 30, 2021
Enable Python 3.10 for conda testing python-control/Slycot#161
Merged
@murrayrm murrayrm added this to the 0.9.1 milestone Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

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

3 participants

@murrayrm @coveralls @bnavigator
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.