Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System norms 2 #971

Merged
merged 6 commits into from
Feb 18, 2024
Merged

System norms 2 #971

merged 6 commits into from
Feb 18, 2024

Conversation

henriks76
Copy link

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

@bnavigator bnavigator mentioned this pull request Feb 18, 2024
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
Copy link
Contributor

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.

Copy link
Author

@henriks76 henriks76 Feb 18, 2024

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

Copy link
Contributor

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.

@bnavigator
Copy link
Contributor

I have fixed the futurewarning test. Please rebase.

Henrik Sandberg added 6 commits February 18, 2024 17:28
* 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.
* Use of warnings package.
* Use routine statesp.linfnorm when slycot installed.
* New routine internal _h2norm_slycot when slycot is installed.
* 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
* In sysnorm_test: Use pytest.warns context manager
* sysnorm_test: Use default tolerance in the tests.
@coveralls
Copy link

Coverage Status

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

@bnavigator bnavigator merged commit e1e33e4 into python-control:main Feb 18, 2024
23 checks passed
@bnavigator
Copy link
Contributor

Thank you @henriks76!

@henriks76 henriks76 deleted the system-norms branch February 19, 2024 09:25
@KybernetikJo
Copy link
Contributor

KybernetikJo commented Feb 21, 2024

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.

@henriks76
Copy link
Author

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

@henriks76
Copy link
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.

@KybernetikJo
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants