GeezerVison

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

GeezerVison is showing github.com — ENH: add `linform` to compute linear system L-infinity norm by roryyorke · Pull Request #729 · python-control/python-control

Conversation

roryyorke commented Apr 24, 2022

Copy link

Copy Markdown

Contributor

linfnorm requires Slycot routine ab13dd, which does the actual work.

Added tests for SISO and MIMO continuous- and discrete-time. MIMO
reference values are generated by linfnorm itself, but results have
been reviewed graphically for correctness.

The MIMO tests rely, unfortunately, on circular logic: the very function being tested has been used to generate test data. I could scour the various textbooks I have for independent examples, but I'm more-or-less assuming that SLICOT's AB13DD is correct, and the tests are to make sure no gross errors have occurred in wrapping. I've tried to cover the various possible cases of gpeak finite (but non-zero) and infinite, and fpeak 0, finite and non-zero, and infinite. I just realized I haven't checked for gpeak = 0; I'll add that (system would have to be non-minimal, or be a static gain of 0; I'll try the former first).

Below are the MIMO test cases for review; everything looks OK to me. The left-hand figures are continuous-time, the right-hand discrete time.

While doing this rather hacky test-generation, I found that StateSpace's repr is not eval-able; this is due, in turn, to numpy.ndarray's repr not being eval-able. I don't know why that is, but I monkey-patched StateSpace.__repr__ to address this. This new repr is eval-able, and I think looks a little better too (look in linfnorm_mimo_testcases.py for examples).

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke commented Apr 24, 2022

Copy link

Copy Markdown

Contributor Author

I don't know how to handle those np.matrix "PendingDeprecation" warnings-as-errors. I see matrixfilter and matrixerrorfilter marks in conftest.py, but I'm not sure when they're supposed to be used.

All reactions

Sorry, something went wrong.

Uh oh!

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

roryyorke commented Apr 24, 2022

Copy link

Copy Markdown

Contributor Author

I am still a bit lost with the PendingDeprecation error from Numpy, which I can't reproduce locally. I developed this PR in a conda environment with Python 3.9.2, pytest 6.2.3 (which I see is a bit old), numpy 1.20.2, scipy 1.6.2, and Slycot 0.4.0.0. python-control tests pass in this environment.

I just created a brand-new conda environment with Python 3.7.12, Pytest 7.1.2, Numpy 1.21.6, Scipy 1.7.3, and Slycot 0.4.0.0. python-control tests all pass in this environment.

I'm running on a 64-bit Intel i5 Ubuntu 20.04 system.

All reactions

Sorry, something went wrong.

Uh oh!

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

bnavigator commented Apr 24, 2022

Copy link

Copy Markdown

Contributor

Your environment should be perfectly fine. I guess you can reproduce locally when you set export PYTHON_CONTROL_ARRAY_AND_MATRIX=1.

The critical part is not to create StateSpace or TransferFunction objects outside of the actual test functions. They are currently created at import time of the module, which is during the test collection.The filter fixtures don't apply yet. You have to move the systems into reference fixtures.

All reactions

Sorry, something went wrong.

Uh oh!

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

roryyorke commented Apr 24, 2022

Copy link

Copy Markdown

Contributor Author

You have to move the systems into reference fixtures.

Thanks, I'll investigate that.

Not being able to reproduce the error locally makes this tricky.

All reactions

Sorry, something went wrong.

Uh oh!

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

coveralls commented Apr 24, 2022 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Coverage increased (+0.006%) to 94.312% when pulling 02b3451 on roryyorke:rory/linfnorm into 46b3c53 on python-control:master.

All reactions

Sorry, something went wrong.

Uh oh!

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

roryyorke commented Apr 24, 2022

Copy link

Copy Markdown

Contributor Author

export PYTHON_CONTROL_ARRAY_AND_MATRIX=1

Oops, missed that. At least it's passing now.

All reactions

Sorry, something went wrong.

Uh oh!

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

murrayrm reviewed

Apr 25, 2022

View reviewed changes

murrayrm left a comment

Copy link

Copy Markdown

Member

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.

Some small comments.

The method of generating test cases is a bit nonstandard. Do we need to check correctness of the algorithm itself (versus the processing of arguments, etc)? Seems like we can assume slycot is working and just do a few simple unit tests??

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/statesp.py Outdated

		-------
		gpeak : non-negative scalar
		L-infinity norm
		fpeak : non-negatie scalar

murrayrm Apr 25, 2022

Copy link

Copy Markdown

Member

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.

negatie → negative

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/statesp.py Outdated

Comment on lines +1921 to +1922

		For systems with an undefined time base (i.e., sys.dt is None), a
		ValueError is raised.

murrayrm Apr 25, 2022

Copy link

Copy Markdown

Member

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.

Normally we allow dt=None to be treat as either continuous or discrete time, with default being continuous. Should we do that here? This is not a major issue since dt=0 by default, but there are situations where a constant system can be converted to a state space or transfer function with dt=None (since it works as either discrete or continuous).

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke Apr 25, 2022

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.

I'll drop the ValueError, and use .isctime() as suggested below; this will treat these as continuous-time.

One case where dt=None by default is static gains (this is from observation, not from reading the code); and, in this case, it doesn't matter what the time base is.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/statesp.py Outdated

Comment on lines +1943 to +1945

		if sys.dt is None:
		raise ValueError('unspecified time base')
		dico = 'C' if sys.dt==0 else 'D'

murrayrm Apr 25, 2022

Copy link

Copy Markdown

Member

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.

Alternatively:

dico = 'C' if sys.isctime() else 'D'

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/statesp.py

Comment on lines +1935 to +1942

		if n == 0:
		# ab13dd doesn't accept empty A, B, C, D;
		# static gain case is easy enough to compute
		gpeak = scipy.linalg.svdvals(d)[0]
		# max svd is constant with freq; arbitrarily choose 0 as peak
		fpeak = 0
		return gpeak, fpeak

murrayrm Apr 25, 2022

Copy link

Copy Markdown

Member

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.

Note that this will return a value even if sys.dt == None, contrary to the documentation.

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke commented Apr 25, 2022

Copy link

Copy Markdown

Contributor Author

The method of generating test cases is a bit nonstandard. Do we need to check correctness of the algorithm itself (versus the processing of arguments, etc)? Seems like we can assume slycot is working and just do a few simple unit tests??

Fair enough, I did get a bit carried away.

We do need to test at least for:

static gains (ab13dd doesn't handle these)
calling ab13dd with correct DICO value for continuous or discrete-time
scaling of frequency for discrete-time systems

What do you think about keeping one continuous-time SISO test (a slightly underdamped system?), an equivalent SISO discrete-time test, and one continuous-time MIMO test (probably duplicating the SISO test as a 2x2 diagonal system) ? Also one static-gain test.

All reactions

Sorry, something went wrong.

Uh oh!

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

roryyorke commented Apr 27, 2022

Copy link

Copy Markdown

Contributor Author

All points addressed. If the tests pass, I'll squash the PR.

All reactions

Sorry, something went wrong.

Uh oh!

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


          ENH: add linform to compute linear system L-infinity norm

02b3451

linfnorm requires Slycot routine ab13dd, which does the actual work.

Added tests to check that wrapping of ab13dd is correct, that static
systems are handled, and that discrete-time system frequencies are
scaled correctly.

roryyorke force-pushed the rory/linfnorm branch from 514e323 to 02b3451 Compare

April 27, 2022 05:38

bnavigator approved these changes

Apr 27, 2022

View reviewed changes

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.

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm merged commit dd95f35 into python-control:master

Apr 27, 2022

Reviewers

murrayrm

murrayrm left review comments

bnavigator

bnavigator approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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.

GeezerVison

Conversation

roryyorke commented Apr 24, 2022

Uh oh!

roryyorke commented Apr 24, 2022

Uh oh!

roryyorke commented Apr 24, 2022

Uh oh!

bnavigator commented Apr 24, 2022

Uh oh!

roryyorke commented Apr 24, 2022

Uh oh!

coveralls commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roryyorke commented Apr 24, 2022

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

murrayrm Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

murrayrm Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

roryyorke Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

murrayrm Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

murrayrm Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

roryyorke commented Apr 25, 2022

Uh oh!

roryyorke commented Apr 27, 2022

Uh oh!

bnavigator left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Apr 24, 2022 •

edited

Loading

Uh oh!

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