You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
sys.dcgain() should return a real value rather than a complex value (until/unless we support complex coefficients) #578
right now sys.dcgain() returns a number that is always something like 10 + 0j. The 0j is extraneous (and makes it a complex number rather than a real number) because, at least currently in python-control, the coefficients of our systems are always real, so the dc gain is a real number.
This happens because dcgain(sys) is just sys(0) and this is a complex number (this came from PR #542). A quick fix would be to just return abs(sys(0)).
Just to check: if we make this change, the DC gain of a system with negative zero frequency gain would be positive (right now the DC gain for the transfer function 1/(s-1) is -1; with this change it would be +1).
re complex-valued systems: presumably we would update dcgain() for such systems once they are supported, to allow for complex values?
re negative-gain systems: seems like there are two options: abs(sys(0)) or sys(0).real. Matlab seems to use the second, giving -1 for dcgain(tf(-1, 1)).
my current take: we should use abs() in the method, and use .real in the matlab layer.
Originally I was thinking that dcgain should correspond to the magnitude at zero. But on further thought, gain does not necessarily mean magnitude. so now I like sys(0).real better.
I'm OK with the proposed approach, but I wonder if we shouldn't also (separately) start thinking about making python-control more pythonic.
Why do we need dcgain(sys)? Isn't sys(0).real as easy (and the same number of characters, even)? And it is more clear because the meaning is not "hidden" behind an anachronistic term (dc = direct current...).
Similarly, why do we have freqresp(sys, omega) when we can just say sys(1j*omega)?
So we could just get rid of some of these extraneous functions and put them all in either control.matlab or create a control.matlab-aliases that defines the "standard" MATLAB commands as lightweight functions that call the more pythonic signatures.
Similarly, why do we have freqresp(sys, omega) when we can just say sys(1j*omega)
One possible reason: if the system is discrete-time, it's cumbersome to type sys(np.exp(1j*omega*sys.dt)) compared to sys.frequency_response(omega).
That said, along those lines of simplifying things, I could be convinced that frequency_response should return just the complex response of the system at the frequencies omega (after a warning if going above the nyquist frequency). Rather than mag, phase, and (sorted) omega as it does now.
Good point on frequency_response for discrete time, though we could still get rid of freqresp as a standalone function (keeping it in the MATLAB compatibility module).
Lti.freqresp is where the matlab layer’s freqresp function is. That’s pretty common to have things all over, not just in matlab.wrappers (eg, same for tf and ss).
Activity
juanodecc commentedon Mar 19, 2021
I agree, I don't know what a complex value mean.
sawyerbfuller commentedon Mar 19, 2021
right now
sys.dcgain()returns a number that is always something like10 + 0j. The0jis extraneous (and makes it a complex number rather than a real number) because, at least currently in python-control, the coefficients of our systems are always real, so the dc gain is a real number.bnavigator commentedon Mar 19, 2021
We still have #484 open. Support for complex StateSpace matrices was requested by users more than once.
Converting the complex value to real in time response tools like step_info like it is right now, makes sense though.
murrayrm commentedon Mar 19, 2021
This happens because
dcgain(sys)is justsys(0)and this is a complex number (this came from PR #542). A quick fix would be to just returnabs(sys(0)).Just to check: if we make this change, the DC gain of a system with negative zero frequency gain would be positive (right now the DC gain for the transfer function 1/(s-1) is -1; with this change it would be +1).
sawyerbfuller commentedon Mar 19, 2021
re complex-valued systems: presumably we would update
dcgain()for such systems once they are supported, to allow for complex values?re negative-gain systems: seems like there are two options:
abs(sys(0))orsys(0).real. Matlab seems to use the second, giving-1fordcgain(tf(-1, 1)).my current take: we should use
abs()in the method, and use.realin the matlab layer.bnavigator commentedon Mar 19, 2021
But why disallow negative gain?
bnavigator commentedon Mar 19, 2021
How about only casting to real if the imaginary part is 0?
https://numpy.org/doc/stable/reference/generated/numpy.isreal.html
sawyerbfuller commentedon Mar 19, 2021
Originally I was thinking that dcgain should correspond to the magnitude at zero. But on further thought, gain does not necessarily mean magnitude. so now I like
sys(0).realbetter.sawyerbfuller commentedon Mar 19, 2021
I like @bnavigator's idea.
bnavigator commentedon Mar 19, 2021
Question: Implement based on current master right now and rebase #577 or merge that first? step_info builds heavily on on
dcgain()sawyerbfuller commentedon Mar 19, 2021
My vote is first option. happy to approve a PR on your suggested DCgain modification.
murrayrm commentedon Mar 19, 2021
I'm OK with the proposed approach, but I wonder if we shouldn't also (separately) start thinking about making python-control more pythonic.
Why do we need
dcgain(sys)? Isn'tsys(0).realas easy (and the same number of characters, even)? And it is more clear because the meaning is not "hidden" behind an anachronistic term (dc = direct current...).Similarly, why do we have
freqresp(sys, omega)when we can just saysys(1j*omega)?So we could just get rid of some of these extraneous functions and put them all in either
control.matlabor create acontrol.matlab-aliasesthat defines the "standard" MATLAB commands as lightweight functions that call the more pythonic signatures.murrayrm commentedon Mar 19, 2021
Also, I don't have MATLAB on my system any more (cutting the cord!), but what does MATLAB return for
dcgain(tf([1], [1, 0])?bnavigator commentedon Mar 19, 2021
😜
sawyerbfuller commentedon Mar 19, 2021
@murrayrm
One possible reason: if the system is discrete-time, it's cumbersome to type
sys(np.exp(1j*omega*sys.dt))compared tosys.frequency_response(omega).That said, along those lines of simplifying things, I could be convinced that
frequency_responseshould return just the complex response of the system at the frequenciesomega(after a warning if going above the nyquist frequency). Rather than mag, phase, and (sorted) omega as it does now.murrayrm commentedon Mar 19, 2021
Good point on
frequency_responsefor discrete time, though we could still get rid offreqrespas a standalone function (keeping it in the MATLAB compatibility module).bnavigator commentedon Mar 19, 2021
Did we simply forget to deprecate
lti.freqresp, when we deprecated the.freqresp()methods for the LTI systems?sawyerbfuller commentedon Mar 20, 2021
Lti.freqresp is where the matlab layer’s freqresp function is. That’s pretty common to have things all over, not just in matlab.wrappers (eg, same for tf and ss).