Skip to content
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

Fix for bug in margin.py #58

Closed
callawaycass opened this issue Apr 16, 2015 · 1 comment
Closed

Fix for bug in margin.py #58

callawaycass opened this issue Apr 16, 2015 · 1 comment
Labels

Comments

@callawaycass
Copy link

Calling margin() or stability_margins() with (mag, phase, omega) form yields incorrect result. I found that the issue is in line 125 of margin.py, where it calls frdata.FRD to create the system:

sys = frdata.FRD(mag*np.exp((1j/360.)*phase), omega, smooth=True)

The phase angle conversion is incorrect. Changing this line to:

sys = frdata.FRD(mag*np.exp(1j*phase*np.pi/180), omega, smooth=True)

yields the correct result. Also, it may be good to clearly indicate that the magnitude data in this form must be absolute magnitude, not dB, and that the phase must be in degrees. The frequency can be either Hz or rad/s... the output frequency is in the same units as whatever you put in (Matlab's documentation has a similar note).

@cwrowley
Copy link
Contributor

Good catch--thanks @callawaycass

Now that you pointed this out, it seems inconsistent for this routine to take phase inputs in degrees, while sys.freqresp(omega) returns phases in radians. For instance, one might expect the following code to work:

sys = TransferFunction(1,[1,1])
mag, phase, omega = sys.freqresp(np.logspace(-1,1,100))
margins = stability_margins((mag,phase,omega))

but the units of phase are wrong. So I wonder if we should change freqresp to return phase in degrees, in order to be more consistent? I notice the docstring for freqresp doesn't specify the units of phase either, so regardless of whether we change the routine, we should improve the documentation.

@cwrowley cwrowley added the bug label Apr 18, 2015
cwrowley added a commit to cwrowley/python-control that referenced this issue Apr 18, 2015
cwrowley added a commit to cwrowley/python-control that referenced this issue Apr 18, 2015
Fixes python-control#58.  The documentation is also updated to specify the units of
the inputs and outputs.  Also, previously there was an optional
argument `deg` which the docstring claimed was used to specify the
units of the input/output phases, but this was never actually used in
the implementation.  This commit removes that optional argument.
cwrowley added a commit to cwrowley/python-control that referenced this issue Apr 18, 2015
cwrowley added a commit to cwrowley/python-control that referenced this issue Apr 18, 2015
Fixes python-control#58.  The documentation is also updated to specify the units of
the inputs and outputs.  Also, previously there was an optional
argument `deg` which the docstring claimed was used to specify the
units of the input/output phases, but this was never actually used in
the implementation.  This commit removes that optional argument.
murrayrm added a commit that referenced this issue Apr 18, 2015
Fix #58, bug in stability_margins
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants