Repository navigation
Fix stability_margins missing a phase crossover at the Nyquist frequency for discrete-time systems - #1271
Closed
gimballock wants to merge 2 commits into
Closed
Fix stability_margins missing a phase crossover at the Nyquist frequency for discrete-time systems#1271gimballock wants to merge 2 commits into
gimballock wants to merge 2 commits into
Conversation
For a discrete-time loop whose phase reaches -180 degrees exactly at w = pi/dt, stability_margins returned gm = inf under every method. For real coefficients L(-1) is always real, so this is a common case: L(z) = 1/(z-1) has a phase crossover at z = -1 with gm = 2, but 'poly', 'frd' and 'best' all reported gm = inf. - _z_filter kept only unit-circle roots with 0 <= arg(z) < pi, dropping the root at z = -1. Now also keep roots whose argument is exactly +pi or exactly -pi (the root-finder returns an exact imaginary zero for a real root, with either sign) and report the endpoint frequency via abs(arg). No tolerance is applied to the angle, so a genuine root near, but not at, -1 keeps its computed frequency and its lower-half conjugate stays excluded, exactly as before. - The 'frd' path detects phase crossovers as sign changes inside the frequency grid, which excludes w = pi/dt for discrete-time systems. When a transfer function is converted to FRD, evaluate L(-1) and count it as a phase crossover if it is real and negative and pi/dt >= epsw. (Other FRD-detected crossings ignore epsw; that pre-existing behavior is unchanged.) This also makes phase_crossover_frequencies include the real-axis intersection at w = pi/dt for discrete-time systems, matching the w = 0 endpoint that was already returned; one existing test reference is updated accordingly. Adds regression tests: gm at the Nyquist endpoint under all three methods, a loop with L(-1) > 0 that must keep gm = inf, and a _z_filter unit test covering the exact endpoint and a nearby conjugate root pair. The new tests fail at baseline and pass with the fix. Fixes python-control#1267
Member
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1267. This resubmits #1268 by @RoboX2020, which was closed before review; its two commits are unchanged.
What it does, for discrete-time systems:
stability_marginschecks z = −1 directly: if L(−1) is real and negative, there's a phase crossover at ω = π/dt with gain margin −1/L(−1).polypath this is a check after the real-axis search, not a change to_z_filter. That filter is shared with the gain-crossover search, and widening it let stability_margins: 'poly' (and default 'best') returns spurious gain crossovers for discrete-time systems of moderate order; the root tolerance eps**(1/n) grows with degree #1266's off-circle roots through (see the discussion on Fix missing discrete-time Nyquist endpoint in stability margins #1268).frdpath, andbest's fallback to it, L(−1) is taken before the conversion to FRD. User-supplied FRD data is unchanged.phase_crossover_frequenciesalso returns the real-axis intersection at π/dt.How it was checked:
main(bfe9380).poly,bestandfrd; onmain, 12 of the 15 returninf.main. The moving averages' are still wrong on both, which is stability_margins: 'poly' (and default 'best') returns spurious gain crossovers for discrete-time systems of moderate order; the root tolerance eps**(1/n) grows with degree #1266, not addressed here.Disclosure: written with an AI agent, Claude Code (Anthropic's Claude Opus 5.5 model). The code is #1268's, by @RoboX2020, unchanged; its author has said it was bot-written. The agent ran the checks above, reviewed the diff and wrote this description. Before posting, it walked me through the change and this description, with a short quiz on the key points: the bug, why the fix checks z = −1 directly, and what it leaves to #1266. I didn't write any of the code.