- Notifications
You must be signed in to change notification settings - Fork446
Add aliases of selected functions as member functions to LTI#1092
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedJan 7, 2025 • 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.
murrayrm left a comment
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.
Thanks for these contributions. These look OK to me.
I think we need to put in some sort of unit test just to make sure the functions work as intended. This is perhaps as simple as just creating a state space system (which has all of the member functions defined), making sure all of the member functions point to the existing function, and then calling it with no argument and with some simple argument(s) to make sure we get aControlPlot back.
| # conversions | ||
| to_ss:Callable | ||
| to_tf:Callable | ||
| # system interconnections | ||
| feedback:Callable | ||
| # freq domain plotting | ||
| bode_plot:Callable | ||
| nyquist_plot:Callable | ||
| nichols_plot:Callable | ||
| # time domain simulation | ||
| forced_response=control.timeresp.forced_response | ||
| impulse_response=control.timeresp.impulse_response | ||
| step_response=control.timeresp.step_response |
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.
We should add (short) docstrings for these. Since these are initialized as assigned member variables instead of standard function (usingdef), we will have to use the doc-comment format (lines starting with #: before the variable). SeeInputOutputSystem.nstates for an example.
I think these can be short, perhaps of the form: "Bode plot; seebode_plot function for more information".
control/tests/kwargs_test.py Outdated
| 'append':test_unrecognized_kwargs, | ||
| 'bode':test_response_plot_kwargs, | ||
| 'bode_plot':test_response_plot_kwargs, | ||
| 'LTI.bode_plot':test_response_plot_kwargs, |
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 would add a comment at the end of this line (and the equivalent lines below) indicating that this is tested viabode_plot since there is not a separate test for the member function (and it really isn't needed, since it is the same object).
…as for feedback and added stub to LTI
lkies commentedJan 10, 2025
Hi, |
a1791c9 intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
And add automatic frequency computation to
LTI.frequency_responsesince refactoring to replace it withcontrol.frequency_responseis not possible without breaking changes.Closes#1084