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 — System norms 2 by henriks76 · Pull Request #971 · python-control/python-control

Conversation

@henriks76

@henriks76 henriks76 commented Feb 17, 2024

Copy link
Copy Markdown

I believe all earlier feedback on sysnorm.py has been addressed now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bnavigator bnavigator mentioned this pull request Feb 18, 2024
System norms #960
Closed
bnavigator
bnavigator reviewed Feb 18, 2024
View reviewed changes
Comment thread control/tests/sysnorm_test.py Outdated
Comment on lines +19 to +20
assert np.allclose(ct.norm(G1, p='inf', tol=1e-9 ), 1.0) # Comparison to norm computed in MATLAB
assert np.allclose(ct.norm(G1, p=2), 0.707106781186547) # Comparison to norm computed in MATLAB

@bnavigator bnavigator Feb 18, 2024

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.

See #960 (comment)

Please don't restrict the tolerance to 1e-9. It will create problems like the one solved in #366.

Sorry, something went wrong.

Uh oh!

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

@henriks76 henriks76 Feb 18, 2024 •
edited
Loading

Uh oh!

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

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

Is it OK to set the default to 1e-2? (For comparison: statesp.linfnorm uses default tolerance 1e-10, and MATLAB's hinfnorm uses 1e-2.)

Sorry, something went wrong.

Uh oh!

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

@bnavigator bnavigator Feb 18, 2024

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 maybe a bit coarse. I'd say we start with 1e-6 and see if we get failures, e.g. in distribution packages like the openSUSE rpm or conda after the next release.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bnavigator

bnavigator commented Feb 18, 2024

Copy link
Copy Markdown
Contributor

I have fixed the futurewarning test. Please rebase.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Henrik Sandberg added 6 commits February 18, 2024 17:28
Henrik Sandberg
New function for LTI system norm computation
a1f27db
Henrik Sandberg
* Updated documentation of function norm
e8cca34 
* Added control/tests/sysnorm_test.py
* Added additional tests and warning messages for systems with poles close to stability boundary
* Added __all__
* Do not track changes in VS Code setup.
Henrik Sandberg
Added:
f60c83a 
* Use of warnings package.
* Use routine statesp.linfnorm when slycot installed.
* New routine internal _h2norm_slycot when slycot is installed.
Henrik Sandberg
Added in sysnorm.py:
99e49d9 
* type check when calling ct.norm
* metod argument in ct.norm (slycot or scipy) using function _slycot_or_scipy from ct.mateqn

Change in sysnorm_test.py
* set print_warning=False
Henrik Sandberg
* In sysnorm: Warnings now of type UserWarning
b39710f 
* In sysnorm_test: Use pytest.warns context manager
Henrik Sandberg
* sysnorm: Changed default tolerance in norm to 1e-6.
bad229a 
* sysnorm_test: Use default tolerance in the tests.
@henriks76
henriks76 force-pushed the system-norms branch from e10b798 to bad229a Compare February 18, 2024 16:34
@coveralls

coveralls commented Feb 18, 2024

Copy link
Copy Markdown

Coverage Status

coverage: 94.422% (-0.4%) from 94.784%
when pulling bad229a on henriks76:system-norms
into af4f8a7 on python-control:main.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator
bnavigator merged commit e1e33e4 into python-control:main Feb 18, 2024
@bnavigator

bnavigator commented Feb 18, 2024

Copy link
Copy Markdown
Contributor

Thank you @henriks76!
henriks76 reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@henriks76
henriks76 deleted the system-norms branch February 19, 2024 09:25
@KybernetikJo

KybernetikJo commented Feb 21, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Thank you @henriks76. I also had plans to implement the norm() method. That's why I was interested in Slycot.ab13bd and Slycot.ab13dd in the first place.

You might want to add the method norm to the function reference in the sphinx documentation. Otherwise, the norm() method is hard to find.

https://github.com/python-control/python-control/blob/main/doc/control.rst
I guess the right place would be Utility functions and conversions after modal_form.

A new PR would be the right way.
bnavigator and henriks76 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

henriks76 commented Feb 23, 2024

Copy link
Copy Markdown
Author

Thanks, @KybernetikJo and @bnavigator, for all the excellent feedback! I'll look into the documentation as soon as possible.
All reactions

Sorry, something went wrong.

Uh oh!

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

henriks76 commented Feb 23, 2024

Copy link
Copy Markdown
Author

Is it correct that the sphinx documentation will be automatically generated from the function docstring, or would you like me to copy it somewhere? This is all very new to me...

I have already tried to follow the conventions here, but let me know if something needs fixing.
All reactions

Sorry, something went wrong.

Uh oh!

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

KybernetikJo commented Feb 23, 2024

Copy link
Copy Markdown
Contributor

Is it correct that the sphinx documentation will be automatically generated from the function docstring, or would you like me to copy it somewhere? This is all very new to me...

No need to copy anything for API documentation, you only have to add your method to
https://github.com/python-control/python-control/blob/main/doc/control.rst

....
modal_form
norm # <-- your method
observable_form
...

The autosummary of sphinx should take care about your API documentation.

You have to install all requirements (https://github.com/python-control/python-control/blob/main/doc/requirements.txt) for sphinx (e.g. with pip)

pip install -r doc/requirements.txt

and then you run the bash command in the doc folder

make html

You can follow the instructions, point 6.
https://github.com/python-control/python-control/wiki/How-to-contribute-with-a-pull-request

I have already tried to follow the conventions here, but let me know if something needs fixing.

I only skimmed over it, but it looks good to me.
We can take care about that later in your PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@henriks76 henriks76 mentioned this pull request Mar 3, 2024
norm added to the sphinx documentation #976
Merged
@bnavigator bnavigator mentioned this pull request Mar 5, 2024
Update to SLICOT v5.9 python-control/Slycot#231
Merged
@murrayrm murrayrm added this to the 0.10.0 milestone Mar 31, 2024
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.10.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

5 participants

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