Repository navigation
lti.evalfr and sys._evalfr have different behavior #434
Description
Activity
We definitely need to remove the inconsistency, but I would encourage us to think more broadly and use a more Pythonic approach:
-
We can override the
__call__function so thatP(s)will represent the "value" of a transfer function at a complex values. This can replace theevalfrfunction. It should work for all LTI classes (including transfer functions, state space systems, FRD systems). -
We can provide a class method
frequency_responsethat takes a variety in input arguments representing a single frequency or list of frequencies. We could shorten this tofreqresp, but I think that using the longer name is more consistent with other usage. We could also provide an "alias" forfreqresp(both as a function and as a method?). -
We can use the MATLAB compatibility module to provide functions that match MATLAB's standard usage, but I don't think we need to represent those in the main
python-controlmodule.
It would be great to get thoughts from others.
Reacted by Ben Greiner-
a
__call__method makes a lot of sense to me. It always struck me as odd how hard it is in matlab to find the frequency response of a transfer function -- I didn't know about evalfr until very recently.Presumably a change moving
evalfrto the matlab module and changingfreqresptofrequency_responsewould entail a version or two in which those were wrappers accompanied by deprecation warnings?Reacted by Ben GreinerShould there be or equivalent ‘call’ for the frequency response of nonlinear iosystems? My first thought is probably not. That ‘call’ should be reserved for the special case (lti systems) where it is useful.
Not clear what a
__call__to a nonlinear input/output system should return. I guess you could pass the state and input and get the derivative and/or output at that state? But if we do that then it returning the frequency response for atltisystem could be confusing (since it is also a state space, I/O system).So I agree with @sawyerbfuller that
__call__should be reserved for special cases where it makes sense and is fairly unambiguous (eg, for LTI systems where it corresponds to the value of the transfer function).Any thoughts from others?
I also agree with all of your considerations. My first thought went into the direction, why use
__call__and not__getitem__? But then I realized this is already used for input/output selection of MIMO systems.IMHO both should work
P(s)[outp:slice, inp:slice]andP[outp:slice, inp:slice](s). What do you think?- What Richard suggests would be useful for numerical simulation of systems (eg #83). Maybe it could be accommodated by something like sys.update(x, u) or sys.xdot(x,u) (not as suitable for discrete time systems), and sys.output(x, u) methods.
A further thought: do you think it would be confusing if
__call__for lti systems was the frequency response, and foriosystemsit returnedxdotandy? The latter can be useful if you want to do direct numerical simulation e.gT = linspace(0,tfinal,dt) for i,t in enumerate(T) xdot, y = sys(x, u(t), t) x = x + dt * xdotIf you wanted to do the same for a state-space system you would just have to turn it into an iosys system.
I think it would be confusing. Also, it is possible for a system to be both and I/O system and an LTI system (there are capabilities in I/O systems, like naming of signals, that you might want to use even if everything it was LTI). So then it would definitely be confusing.
Instead, we might create methods for I/O systems to return the derivative and output (like
_rhsand_out, but not private). I likeupdateandoutput, as you suggested above. (We could also create those forStateSpacesystems, I suppose.)re methods useful for simulating systems: Sounds reasonable.
Before I work on a PR to fix
evalfr/frequency_responseand perhaps add a__call__method, one other question:- the
sys.evalfrmethod gives a deprecationWarning, indicating that one should instead use theevalfrfunction in theltimodule. Anybody know why that is?evalfr/frequency_response, seems to me to be a nearly canonical example of something that should be handled by a method.
- the
OK that makes sense. After thinking it over for awhile, I really like the idea of having a pythonic
__call__method for LTI systems. What does everybody think about the following course of action:- rename
sys._evalfrto besys. __call__, but change it so that its argument is complex valued, to be consistent with the idea that you are evaluating transfer functionP(s)at a complex frequencysas suggested by @murrayrm. This is a change from old behavior, but is ok becausesys._evalfris a private method, right? - it's been 2.5 years since
sys.evalfrwas deprecated. Ok if we make good on the threat and remove it fromTransferFunction, StateSpace, and FrequencyResponseDataclasses now? - add
sys.frequency_responseas a new method that behaves the same as the oldsys.freqresp, and make the latter an alias to the former for the matlab compatibility module - give a deprecation warning for
sys.freqresp, pointing instead tolti.freqresp(sys, omega)
(following the process for howsys.evalfrwas deprecated) - add a new legacy setting
control.evalfr_methodthat, if true, reinstates thesys.evalfrsomehow.
Remarks:
- Note that I propose leaving
FrequencyResponseDatasystems as they are, without a__call__method. Currently they have anevalmethod that takes a real-valued frequency argument. Comments in the code indicate this was done to avoid ambiguity in how the argument should be interpreted. The alternative is a__call__method that can handle either purely real or purely imaginary arguments but raises an error on complex arguments, but I think this could lead to problems if you are expecting the response to a real-valuedsargument. - update code as necessary to in accommodate discrete-time sytems and unit tests, and update e.g.
marginsto use newfrequency_responsemethod
- rename
- rename
sys._evalfrto besys. __call__, but change it so that its argument is complex valued, to be consistent with the idea that you are evaluating transfer functionP(s)at a complex frequencysas suggested by @murrayrm. This is a change from old behavior, but is ok becausesys._evalfris a private method, right?
👍
- it's been 2.5 years since
sys.evalfrwas deprecated. Ok if we make good on the threat and remove it fromTransferFunction, StateSpace, and FrequencyResponseDataclasses now?
'now' being 0.9: 👍
- add
sys.frequency_responseas a new method that behaves the same as the oldsys.freqresp, and make the latter an alias to the former for the matlab compatibility module
👍
- give a deprecation warning for
sys.freqresp, pointing instead tolti.freqresp(sys, omega)
(following the process for howsys.evalfrwas deprecated)
👍
- add a new legacy setting
control.evalfr_methodthat, if true, reinstates thesys.evalfrsomehow.
If it affects just the use of one function, which can be replaced by another function call, I don't think a legacy setting is necessary. Downstream code has to be changed anyway. Instead of introducing the setting to legacy defaults, they can just replace the
evalfrfunction with the appropriate call. Much easier than dealing with changing return types ndarray/matrix.Remarks:
- Note that I propose leaving
FrequencyResponseDatasystems as they are, without a__call__method. Currently they have anevalmethod that takes a real-valued frequency argument. Comments in the code indicate this was done to avoid ambiguity in how the argument should be interpreted. The alternative is a__call__method that can handle either purely real or purely imaginary arguments but raises an error on complex arguments, but I think this could lead to problems if you are expecting the response to a real-valuedsargument.
It is new functionality. Document it properly and it should be fine. I would rather have FRD objects behave in the same way as TF and SS objects in that regard other than creating a special case.
- update code as necessary to in accommodate discrete-time sytems and unit tests, and update e.g.
marginsto use newfrequency_responsemethod
👍
- rename
Following @bnavigator's thoughts, I think it makes sense to add
__call__forfrdsystems with a call signature equivalent to other lti systems, with the exception that it would raise an error for non-imaginarys.Any other thoughts on simply removing
sys.evalfraltogether rather than providing a legacy setting you can use to reinstate it?- linked a pull request that will close this issueuse __call__ instead of evalfr in lti system classes #449
on Aug 16, 2020
The
evalfrfunction in theltimodule expects a complex number and evaluates the system at that complex frequency. The methodsys._evalfr, on the other hand, where sys is a transfer function or state space system, expects a real argument and multiplies it by j before evaluating the frequency response.Is this dichotomy intentional?
evalfrfunction expects a complex number, which seems like a more general-purpose solution.sys.freqrespexpects real-valued omega arguments, making it perhaps a better interface for userssys._evalfrcorrectly performs the z-plane transformation for discrete-time systems.