Repository navigation
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
|
Thanks for the quick fix. I ran it against a dozen discrete-time loops, from first order up to order ~1,350 (moving averages, EWMAs and blockchain difficulty-retarget rules), comparing with a dense frequency scan. With this branch every gain margin matches under That one is an interaction with #1266, through import warnings
import numpy as np
import control as ct
warnings.simplefilter("ignore")
for N in (16, 30, 72, 144):
num = np.r_[0.0, np.full(N, 1.0 / N)]
L = ct.tf(num, np.polysub(np.r_[1.0, np.zeros(N)], num), dt=1)
gm, pm, sm, wpc, wgc, wms = ct.stability_margins(L, method="poly")
print(N, round(pm, 3), round(wgc, 4), abs(complex(L(-1))))
Both are wrong, so this isn't a regression in kind, but 0° reads as a marginally stable loop. Two ways to keep this PR's fix without it: keep z = -1 only in the phase-crossover filter ( |
|
Thanks for catching this. I reproduced the false zero-degree phase margins for N = 16, 30, 72 and 144. The follow-up commit restores The four moving-average phase margins now match Added a moving-average regression for the false Nyquist gain crossover. Margin tests: 74 passed, 2 skipped. Margin, FRD, transfer-function and LTI tests together: 428 passed, 129 skipped. |
|
@RoboX2020 Please add AI tool disclosures, ensuring consistency with https://numpy.org/doc/stable/dev/ai_policy.html ? (Given how long and verbose your responses are and how quickly you are committing, I assume you used a chat bot for most or all of this...) |
|
Disclosure for my comment above: it was written with an AI agent, Claude Code (Anthropic's Claude Opus 5.5 model).
|
|
Thanks @gimballock I guess @RoboX2020 is a bot who simply closed the PR in response to request... If you want, @gimballock , feel free to review this or open a PR that re-uses the code for this PR. I assume @RoboX2020 did not author any of it, so you can just claim authorship. |
Fixes #1267.
For a discrete-time loop with a negative real response at z = -1, the Nyquist endpoint is a phase crossover. For example,
1 / (z - 1)has gain margin 2 atpi / dt, butstability_marginsonmainreturns infinity withpoly,frd, andbest.This evaluates the response at z = -1 directly for discrete-time phase crossovers, in both the polynomial path and transfer-function conversion to FRD, honoring
epswfor the added endpoint.phase_crossover_frequenciesalso includes the real-axis Nyquist intersection. The shared root filter and gain-crossover search are unchanged from upstream.The first version widened the shared root filter. As the review identified, that interacted with #1266 and introduced false zero-degree phase margins for even-length moving averages. The follow-up commit removes that interaction and adds a regression. Their phase margins now match
main, but the existing numerical inaccuracy tracked in #1266 remains out of scope.Tests also cover the three reported loops under all three methods and a positive-real endpoint that must not become a phase crossover.
Validation:
This does not change endpoint handling for user-supplied FRD data or the existing
epswbehavior for crossings found inside an FRD grid.