-
Notifications
You must be signed in to change notification settings - Fork 438
sys.dcgain() should return a real value rather than a complex value (until/unless we support complex coefficients) #578
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
Comments
I agree, I don't know what a complex value mean. |
right now |
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. |
This happens because 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 re negative-gain systems: seems like there are two options: my current take: we should use |
But why disallow negative gain? |
How about only casting to real if the imaginary part is 0? return retval.real if np.all(np.isreal(retval)) else retval https://numpy.org/doc/stable/reference/generated/numpy.isreal.html |
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 |
I like @bnavigator's idea. |
Question: Implement based on current master right now and rebase #577 or merge that first? step_info builds heavily on on |
My vote is first option. happy to approve a PR on your suggested DCgain modification. |
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 Similarly, why do we have So we could just get rid of some of these extraneous functions and put them all in either |
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])
↑
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 😜 |
One possible reason: if the system is discrete-time, it's cumbersome to type That said, along those lines of simplifying things, I could be convinced that |
Good point on |
Did we simply forget to deprecate |
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). |
No description provided.
The text was updated successfully, but these errors were encountered: