GeezerVison is showing github.com — sys.dcgain() should return a real value rather than a complex value (until/unless we support complex coefficients) · Issue #578 · python-control/python-control
sys.dcgain() should return a real value rather than a complex value (until/unless we support complex coefficients) #578Activityjuanodecc commented on Mar 19, 2021 I agree, I don't know what a complex value mean. sawyerbfuller commented on Mar 19, 2021 ContributorAuthor More actions 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. bnavigator commented on 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 commented on Mar 19, 2021 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). sawyerbfuller commented on Mar 19, 2021 ContributorAuthor More actions 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. bnavigator commented on Mar 19, 2021 But why disallow negative gain? bnavigator commented on Mar 19, 2021 sawyerbfuller commented on Mar 19, 2021 ContributorAuthor More actions 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. sawyerbfuller commented on Mar 19, 2021 ContributorAuthor More actions bnavigator commented on 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 commented on Mar 19, 2021 ContributorAuthor More actions My vote is first option. happy to approve a PR on your suggested DCgain modification. murrayrm commented on 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'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. murrayrm commented on 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 commented on 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])?
>> dcgain(tf([1],[1, 0])
dcgain(tf([1],[1, 0])
↑
Error: Invalid expression. When calling a function or indexing a variable, use
parentheses. Otherwise, check for mismatched delimiters.
Did you mean:
>> dcgain(tf([1],[1, 0]))
ans =
Inf😜 sawyerbfuller commented on Mar 19, 2021 ContributorAuthor More actions @murrayrm
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. murrayrm commented on Mar 19, 2021 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). bnavigator commented on Mar 19, 2021 Did we simply forget to deprecate lti.freqresp, when we deprecated the .freqresp() methods for the LTI systems? sawyerbfuller commented on Mar 20, 2021 ContributorAuthor More actions 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).
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|