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 margin() documentation to address issue #195 #198

Merged
merged 3 commits into from Jul 2, 2018

Conversation

murrayrm
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Feb 24, 2018

Coverage Status

Coverage remained the same at 77.882% when pulling 9fd6ae7 on murrayrm:fix_margins_doc into 601b581 on python-control:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.882% when pulling 9fd6ae7 on murrayrm:fix_margins_doc into 601b581 on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.882% when pulling 9fd6ae7 on murrayrm:fix_margins_doc into 601b581 on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.882% when pulling 9fd6ae7 on murrayrm:fix_margins_doc into 601b581 on python-control:master.

@coveralls
Copy link

coveralls commented Feb 24, 2018

Coverage Status

Coverage increased (+0.5%) to 78.395% when pulling 1593391 on murrayrm:fix_margins_doc into 601b581 on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.882% when pulling 9fd6ae7 on murrayrm:fix_margins_doc into 601b581 on python-control:master.

@murrayrm murrayrm mentioned this pull request Mar 11, 2018
Wcp : float
Phase crossover frequency (corresponding to gain margin) (in rad/sec)
wg: float
Gain margin crossover frequency (where phase crosses -180 degrees)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not omit "crossover" altogether? The parenthetical remarks are helpful -- I always get these mixed up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit that the Mathworks explanation is nice here https://www.mathworks.com/help/control/ref/margin.html

I don't agree with the terminology as you now propose.

Gain crossover frequency is where gain is (crosses!) 0dB -- corresponds to gain margin
we can call it Wgco or Wpm

Phase crossover (frequency) is where phase crosses -180 deg -- corresponds to phase margin
We can call it Wpco or wg

Gain margin crossover is simply confusing

Maybe say:
wg: frequency at gain margin (phase crossover point, phase = -180 deg)
wp: frequency at phase margin (gain crossover point, gain = 0dB)

@murrayrm murrayrm added this to the 0.8.0 milestone Jun 30, 2018
@repagh
Copy link
Member

repagh commented Jul 2, 2018

@murrayrm, I added a commit on this PR. I also edited comments for the related stability_margins function.

@murrayrm murrayrm merged commit 5158a39 into python-control:master Jul 2, 2018
@murrayrm murrayrm deleted the fix_margins_doc branch July 2, 2018 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants