lti.evalfr and sys._evalfr have different behavior

The evalfr function in the lti module expects a complex number and evaluates the system at that complex frequency. The method sys._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?

matlab's evalfr function expects a complex number, which seems like a more general-purpose solution. sys.freqresp expects real-valued omega arguments, making it perhaps a better interface for users
also, right now, only sys._evalfr correctly performs the z-plane transformation for discrete-time systems.

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 that P(s) will represent the "value" of a transfer function at a complex value s. This can replace the evalfr function. It should work for all LTI classes (including transfer functions, state space systems, FRD systems).
We can provide a class method frequency_response that takes a variety in input arguments representing a single frequency or list of frequencies. We could shorten this to freqresp, but I think that using the longer name is more consistent with other usage. We could also provide an "alias" for freqresp (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-control module.

It would be great to get thoughts from others.

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 evalfr to the matlab module and changing freqresp to frequency_response would entail a version or two in which those were wrappers accompanied by deprecation warnings?

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

@sawyerbfuller

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 at lti system 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] and P[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 for iosystems it returned xdot and y? The latter can be useful if you want to do direct numerical simulation e.g

T = linspace(0,tfinal,dt) for i,t in enumerate(T) xdot, y = sys(x, u(t), t) x = x + dt * xdot

If 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 _rhs and _out, but not private). I like update and output, as you suggested above. (We could also create those for StateSpace systems, I suppose.)

re methods useful for simulating systems: Sounds reasonable.

Before I work on a PR to fix evalfr/frequency_response and perhaps add a __call__ method, one other question:

the sys.evalfr method gives a deprecationWarning, indicating that one should instead use the evalfr function in the lti module. 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.

See issue #22 for some discussion on this (including many of the things we are discussing above -:).

@murrayrm

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._evalfr to be sys. __call__, but change it so that its argument is complex valued, to be consistent with the idea that you are evaluating transfer function P(s) at a complex frequency s as suggested by @murrayrm. This is a change from old behavior, but is ok because sys._evalfr is a private method, right?
it's been 2.5 years since sys.evalfr was deprecated. Ok if we make good on the threat and remove it from TransferFunction, StateSpace, and FrequencyResponseData classes now?
add sys.frequency_response as a new method that behaves the same as the old sys.freqresp, and make the latter an alias to the former for the matlab compatibility module
give a deprecation warning for sys.freqresp, pointing instead to lti.freqresp(sys, omega)
(following the process for how sys.evalfr was deprecated)
add a new legacy setting control.evalfr_method that, if true, reinstates the sys.evalfr somehow.

Remarks:

Note that I propose leaving FrequencyResponseData systems as they are, without a __call__ method. Currently they have an eval method 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-valued s argument.
update code as necessary to in accommodate discrete-time sytems and unit tests, and update e.g. margins to use new frequency_response method

@murrayrm

rename sys._evalfr to be sys. __call__, but change it so that its argument is complex valued, to be consistent with the idea that you are evaluating transfer function P(s) at a complex frequency s as suggested by @murrayrm. This is a change from old behavior, but is ok because sys._evalfr is a private method, right?

👍

it's been 2.5 years since sys.evalfr was deprecated. Ok if we make good on the threat and remove it from TransferFunction, StateSpace, and FrequencyResponseData classes now?

'now' being 0.9: 👍

add sys.frequency_response as a new method that behaves the same as the old sys.freqresp, and make the latter an alias to the former for the matlab compatibility module

👍

give a deprecation warning for sys.freqresp, pointing instead to lti.freqresp(sys, omega)
(following the process for how sys.evalfr was deprecated)

👍

add a new legacy setting control.evalfr_method that, if true, reinstates the sys.evalfr somehow.

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 evalfr function with the appropriate call. Much easier than dealing with changing return types ndarray/matrix.

Remarks:

Note that I propose leaving FrequencyResponseData systems as they are, without a __call__ method. Currently they have an eval method 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-valued s argument.

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. margins to use new frequency_response method

👍

@bnavigator

Following @bnavigator's thoughts, I think it makes sense to add __call__ for frd systems with a call signature equivalent to other lti systems, with the exception that it would raise an error for non-imaginary s.

Any other thoughts on simply removing sys.evalfr altogether rather than providing a legacy setting you can use to reinstate it?

Navigation Menu

Repository navigation

lti.evalfr and sys._evalfr have different behavior #434

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

lti.evalfr and sys._evalfr have different behavior #434

Description

Activity

murrayrm commented on Jul 23, 2020

sawyerbfuller commented on Jul 23, 2020

sawyerbfuller commented on Jul 25, 2020

murrayrm commented on Jul 25, 2020

bnavigator commented on Jul 25, 2020

sawyerbfuller commented on Jul 25, 2020

sawyerbfuller commented on Jul 27, 2020

murrayrm commented on Jul 27, 2020

sawyerbfuller commented on Jul 27, 2020

murrayrm commented on Jul 27, 2020

sawyerbfuller commented on Jul 28, 2020

bnavigator commented on Jul 28, 2020

sawyerbfuller commented on Jul 29, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions