- Notifications
You must be signed in to change notification settings - Fork441
System norms 2#971
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
System norms 2#971
Uh oh!
There was an error while loading.Please reload this page.
Conversation
control/tests/sysnorm_test.py Outdated
| 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 |
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.
Please don't restrict the tolerance to 1e-9. It will create problems like the one solved in#366.
henriks76Feb 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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'shinfnorm uses 1e-2.)
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.
I have fixed the futurewarning test. Please rebase. |
* 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.
* type check when calling ct.norm* metod argument in ct.norm (slycot or scipy) using function _slycot_or_scipy from ct.mateqnChange 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 commentedFeb 18, 2024
Thank you@henriks76! |
KybernetikJo commentedFeb 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 https://github.com/python-control/python-control/blob/main/doc/control.rst A new PR would be the right way. |
Thanks,@KybernetikJo and@bnavigator, for all the excellent feedback! I'll look into the documentation as soon as possible. |
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 conventionshere, but let me know if something needs fixing. |
No need to copy anything for API documentation, you only have to add your method to 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.
I only skimmed over it, but it looks good to me. |
I believe all earlier feedback on
sysnorm.pyhas been addressed now.