Skip to content

Response Functions as Member Functions? #1084

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

Closed
lkies opened this issue Dec 23, 2024 · 3 comments · Fixed by #1092
Closed

Response Functions as Member Functions? #1084

lkies opened this issue Dec 23, 2024 · 3 comments · Fixed by #1092

Comments

@lkies
Copy link
Contributor

lkies commented Dec 23, 2024

Hi,
are there some ideological reasons why the response functions are not available as member functions for the respective system classes like they are in sicpy.signal? I feel like this would make the library a bit nicer to use.
I am willing to implement this myself, I just want to ensure that it would actually have a change of getting merged before putting in the effort.
Just to be clear, I mean forwarding the calls or aliasing the methods, not refactoring the code.

@murrayrm
Copy link
Member

Seems easy enough to implement. Note that frequency_response is already a method for all LTI systems, so the following works to generate a Bode plot:

import numpy as np
import control as ct
sys = ct.rss(4, 1, 1)
sys.frequency_response(np.linspace(-3, 3)).plot()

One thing that is missing from the sys.frequency_response() method versus the ct.frequency_response() function is automatic computation of the frequency range. So that would be something to fix/update as well.

Time respones (forced_response, impulse_response, initial_response, input_output_response, step_response) could be implemented fairly simply, as you suggest.

If you or someone else wants to take a pass at this, it would be a nice contribution. It will require some refactoring, at least for frequency_response since right now ct.frequency_response computes the default frequency range and then calls the class method. Also, I think that initial_response should only be allowed on systems with an explicit state (so not transfer functions), since there is no well-defined notion of the initial state.

@lkies
Copy link
Contributor Author

lkies commented Dec 26, 2024

Thank you for quick response and the hints regarding frequency_response. I will take a closer look and get back to you / this thread.

@lkies
Copy link
Contributor Author

lkies commented Jan 7, 2025

Ok, i have finally found some time for this.

Refactoring it so that LTI.frequency_respose = frequency_response (the free one) and moving the old LTI.frequency_response into the body of frequency_response almost works. the only difference in behavior seems to be, that the free function interprets omega=[a, b] as the range from a to b where the member function interprets it as just the frequencies [a, b]. I don't see a reasonable way of handling this without breaking changes, so I would just leave it for now.

I think it would be a good idea to deprecate passing limits in omega as opposed to omega_limits (at least for everything that is not a tuple) so this could be implemented in some future version.

I implemented the automatic frequency range determination for the frequency_response member function but i do not think implementing the other features that require adding extra arguments is a good idea since this would make the two function very similar except for this specific edge case which would make them awkward / confusing to use. Maybe implementing automatic frequency range determination is already a step too far, but since the signature allows not passing omega which will lead to an error somewhere in the body it has some justification to actually make it optional.

Other than that I added every function that seemed sensible to me and that can take a system as its first argument as a member to LTI. But it turned out more complicated than anticipated because of cyclical imports. Since the modules that implement these functions all directly and indirectly depend on lti.py they can not simply be imported in lti.py. It should technically be possible to refactor them all to only import things from lti.py in the function bodies but that would require a lot of changes across a lot of files. The only other work around that I could come up with is forward declaring the names and then patching them in the __init__.py. I am not too happy about that but VSCode (and probably other editors/IDEs) seem to be able to show the docstring.

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 a pull request may close this issue.

2 participants