Add scipy fallbacks for generalized Lyapunov and discrete Sylvester equations - #1234
Conversation
2c68e3e to
c1ab92c
Compare
|
@kwlee2025cpp Thanks for proposing these changes. Can you answer a few questions to help with review and consideration for merging?
I am a little concerned that this PR is just a Claude-generated thing that no one is asking for. I am also concerned we are about to get many more Claude-generated PRs as your description suggestions ("...It is the first of a |
|
Approving the workflows because the code on first glance looks good and obviously not malicious. No assessment about scientific merit, I fully support @slivingston's inquiry. |
|
Thanks @slivingston, and @bnavigator — these are the right questions, and I would rather answer them squarely than let this sit as an unmotivated patch. 1. Prior issue / motivation. It is the project's own direction. In #27 ("Move One more disclosure, a personal thread: in 2017 (as @beachdweller) I was on #27 thinking about translating the SLICOT Fortran myself — the same year #133 added a no-Slycot test run. That effort and #27's pointed the same way but never met; I wish I had carried it through then instead of going quiet — belated apologies. The viable road was not translation but these scipy fallbacks. 2. Worth it? — let me be straight, because a survey moved under me.
3. Am I using it. Not in a production project that calls these specific functions today, and I will retract part of my own pitch: I would have cited Fortran-free robotics-grader containers (RoboRacer / F1TENTH on ROS 2), but 0.7.0's On the "Claude-generated" concern. Claude assisted — implementation from one model, review from another, and the commit trailers say so; I am not hiding it. But the motivation, the mathematics, and the direction are mine: I have been through every line — the E ≠ I congruence, the Bartels–Stewart back-substitution and its λ_i·μ_j ≠ 1 solvability condition, the stability argument — and can defend each. I stand behind every line. If a reviewer finds one I cannot account for, that is a bug and I want to hear it. On "many more PRs." It is a finite, bounded effort, sliced small to make review easier, not to flood you — I am glad to pace it, hold the rest until this one is judged, or stop. And knowing the honest and modest case for it, if you would still rather not carry it, I am genuinely happy to withdraw — I do not want to add maintenance you did not ask for. No hard feelings either way. |
slivingston
left a comment
There was a problem hiding this comment.
@kwlee2025cpp Thanks for your reply. Some of it is unclear, but I think the only part critical for review is
Performance: compiled SLICOT should win at large n; the scipy path is
LAPACK-backed so it is not slow, but I am not claiming it beats SLICOT — happy to
benchmark.
Yes, please benchmark. This will also be useful for the other PRs that you plan to make/submit.
|
Thanks @slivingston — benchmark (scipy vs slycot across In the meantime, so I fix the unclear parts rather than guess at them: which sections read as unclear to you? I will tighten or rewrite them — I know the reply ran long. |
|
Benchmark — generalized continuous Lyapunov —
|
| n | scipy (ms) | slycot (ms) | scipy/slycot |
|---|---|---|---|
| 10 | 0.09 | 0.07 | 1.27 |
| 20 | 0.19 | 0.26 | 0.75 |
| 50 | 1.0 | 4.6 | 0.22 |
| 100 | 5.8 | 22.8 | 0.26 |
| 200 | 20.9 | 161 | 0.13 |
| 400 | 90.7 | 1003 | 0.09 |
| 800 | 797 | 6780 | 0.12 |
generalized discrete Lyapunov — dlyap(A, Q, E=E)
| n | scipy (ms) | slycot (ms) | scipy/slycot |
|---|---|---|---|
| 10 | 0.15 | 0.09 | 1.72 |
| 20 | 0.24 | 0.32 | 0.75 |
| 50 | 1.6 | 4.5 | 0.36 |
| 100 | 6.7 | 24.8 | 0.27 |
| 200 | 23.5 | 181 | 0.13 |
| 400 | 100 | 1109 | 0.09 |
| 800 | 908 | 7215 | 0.13 |
discrete Sylvester — dlyap(A, Q, C)
| n | scipy (ms) | slycot (ms) | scipy/slycot |
|---|---|---|---|
| 10 | 0.27 | 0.03 | 9.78 |
| 20 | 0.68 | 0.10 | 6.68 |
| 50 | 8.6 | 1.7 | 5.14 |
| 100 | 45.2 | 10.2 | 4.42 |
| 200 | 116 | 56.9 | 2.05 |
| 400 | 598 | 352 | 1.70 |
| 800 | 5187 | 2728 | 1.90 |
Residuals are at machine precision (~1e-15…1e-14) for both methods on all three routines; scipy and slycot agree to ~1e-14 (≤ 1e-9 at n = 800, reflecting the solution's sensitivity at that size).
Headline — I was wrong that SLICOT wins at large n; it depends on the routine:
- Generalized Lyapunov (continuous and discrete): scipy is faster, ~8–11× at n ≥ 400. The scipy path reduces via
inv(E)to a standard Lyapunov equation (LAPACK Bartels–Stewart);SG03ADsolves the generalized equation via generalized Schur, which costs more. - Discrete Sylvester: slycot is faster, ~1.7–10× (widest at small n). My Bartels–Stewart Schur-factors both
AandQᵀplus a Python column loop;SB04QD's Hessenberg–Schur in Fortran is leaner.
Accuracy vs cond(E) — lyap, n = 100, median over 5 seeds, err = ‖X̂ − X_true‖ / ‖X_true‖:
| cond(E) | scipy err | slycot err | slycot / scipy |
|---|---|---|---|
| 1e0 | 7e-15 | 7e-15 | 0.99 |
| 1e2 | 1.1e-13 | 8.5e-14 | 0.80 |
| 1e4 | 2.6e-10 | 3.4e-10 | 1.31 |
| 1e6 | 1.2e-6 | 1.7e-6 | 1.61 |
| 1e8 | 7.6e-3 | 1.5e-2 | 1.95 |
| 1e10 | 67 | 110 | 1.71 |
| 1e12 | 5.2e5 | 9.2e5 | 1.82 |
This one corrected me on my own docstring. I had written that method='slycot' is "preferable" for ill-conditioned E. It isn't: both methods degrade together — the generalized Lyapunov problem is itself ill-conditioned (about cond(E)²) when E is — and slycot is in fact 1.3–2× less accurate for cond(E) ≥ 1e4, never better. I've pushed a commit fixing the docstrings and the runtime warning accordingly.
Net: the fallback is correct everywhere and as accurate as slycot across the whole conditioning range; for generalized Lyapunov it's also a speedup, while for discrete Sylvester slycot remains the faster path when it's installed. (Happy to add the benchmark script to the PR or a gist if useful.)
|
@kwlee2025cpp Thanks for comparing performance. In this PR, can you add the code for replicating the experiment? I think the best path is in benchmarks/. Give the file a name like |
Add benchmarks/scipy_fallback_bench.py (asv) comparing the pure-scipy fallbacks with slycot for generalized Lyapunov (continuous and discrete) and discrete Sylvester, plus an accuracy-vs-cond(E) track for the generalized Lyapunov solution. slycot parameterizations are skipped when slycot is not installed. Requested in the PR python-control#1234 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done — added
Run with: or, since these are plain classes, call the One note on reproducibility: the tables I posted earlier were medians over several seeds, whereas the committed benchmark fixes a single seed per problem so runs are deterministic and comparable across commits (asv's repeated sampling averages the timing). Same generators, same ratios. I ran the suite locally with asv (existing-environment mode) and it reproduces both the timing and the accuracy-vs-cond(E) tables. Happy to adjust the size grid or fold in more routines as the fallbacks grow. |
|
Thanks @kwlee2025cpp I will review it tomorrow. |
|
Hi @slivingston — no rush at all, just a friendly check-in on this one. The benchmark is in as In case the red CI is what's giving pause: those failures are pre-existing and unrelated to this PR — Happy to rebase or adjust whenever you have a chance — thanks for taking a look. |
Thanks for checking in. Sorry I have been slow to reply. I will be done soon. |
|
@kwlee2025cpp Almost done with review. Quick question: Can you tell me the relationship between your account (@kwlee2025cpp ) and @kangwonlee ? I ask because the accounts have similar names and one has forks of the other's repositories. |
|
Both accounts are mine. @kangwonlee is my long-standing personal account, and it owns the fork this PR is branched from. @kwlee2025cpp is the account I use for work where I am getting help from an LLM (Claude). I set it up to be upfront about that, rather than let such work blend into my personal account's history. The commits here are authored under my own name and email, and the pushes use @kwlee2025cpp's SSH key, which has write access to that fork — that is why one account appears to hold forks of the other's repositories. That is the same disclosure I made earlier in this thread: I use Claude's help, I verify the mathematics myself, and I stand behind every line. Happy to answer anything further. I would rather not re-open this PR from the other account, since that would lose your review thread, but I can consolidate for future contributions if you prefer. |
|
FYI: CI failures have been addressed in #1243. Once that has been merged into main, you should be able to rebase and clear the errors. |
|
lgtm, after a rebase |
…quations lyap and dlyap previously raised ControlArgument for method='scipy' (explicit, or auto-selected when slycot is absent) on three cases that SLICOT handles. Add pure scipy/numpy fallbacks for all three: - Generalized Lyapunov, continuous and discrete (E != I): congruence transform by inv(E) to a standard Lyapunov equation, then scipy's solve_continuous/discrete_lyapunov. Requires E nonsingular; SLICOT sg03ad (Penzl's generalized Schur method) also handles singular E and remains the method='slycot' path. A nonsingular-E failure raises a clear ControlArgument. Because the transform inverts E, an ill-conditioned E yields reduced accuracy; this now emits a UserWarning recommending method='slycot' (the continuous path was previously silent, while the discrete path warned only incidentally). - Discrete Sylvester (A X Q^T - X + C = 0): Bartels-Stewart method via complex Schur factors of A and Q^T with column-by-column triangular solves, O(n^3 + m^3), matching the Hessenberg-Schur cost of SLICOT sb04qd. Includes the solvability check that no eigenvalue pair of A and Q^T has product (almost) equal to 1. Also fix two incorrect slycot import-fallback aliases (sb0qmd -> sb04qd, sb04ad -> sg03ad) so the names resolve to None, not NameError, when slycot is absent; and correct the dlyap Notes, which described the scipy Sylvester path as a Kronecker O((nm)^3) solve (it is now Bartels-Stewart, O(n^3 + m^3)). Tests parametrize method=[None, 'scipy', 'slycot'] and cross-check that the scipy and slycot solutions agree, and cover the nonsingular-E requirement, the ill-conditioned-E warning, and the singular discrete-Sylvester case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The generalized-Lyapunov Notes, the scipy-path comments, and the nonsingular-E error messages stated that SLICOT SG03AD "also handles singular E". It does not: SG03AD factors the matrix pencil without inverting E (its advantage for a nonsingular but ill-conditioned E), but a truly singular (descriptor) E returns a degenerate-pair warning and is out of scope for both the scipy and slycot paths. Correct all six spots (continuous lyap and discrete dlyap) to say so. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The generalized-Lyapunov Notes and the _warn_ill_conditioned_E helper (its docstring and warning message) claimed method='slycot' is "preferable" / "more robust" than method='scipy' for ill-conditioned E. Benchmarking (scipy vs slycot, n=100, cond(E) swept 1 to 1e12 over 5 seeds) shows both methods lose accuracy at the same rate -- slycot is in fact 1.3-2x *less* accurate for cond(E) >= 1e4, never better. The generalized Lyapunov problem is itself ill-conditioned (about cond(E)^2) when E is, so no method beats that floor. Reword to say both degrade and slycot is not measurably more accurate, and drop the false "use method='slycot' for a more robust solution" line from the warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add benchmarks/scipy_fallback_bench.py (asv) comparing the pure-scipy fallbacks with slycot for generalized Lyapunov (continuous and discrete) and discrete Sylvester, plus an accuracy-vs-cond(E) track for the generalized Lyapunov solution. slycot parameterizations are skipped when slycot is not installed. Requested in the PR python-control#1234 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6492257 to
2dabe33
Compare
|
Rebased onto main, which now includes #1243 — thanks @murrayrm for the CI fix and @sawyerbfuller for the review. The four commits replay unchanged; the patch is byte-identical to the pre-rebase one. Locally on numpy 2.4.4 / scipy 1.17.1 / matplotlib 3.10.8, The workflow runs on the new head are showing as |
|
@slivingston Do you have any pending comments on this PR? If not, I'll merge as soon as CI checks clear. |
|
Yes, I will wrap it up tonight and merge.
|
slivingston
left a comment
There was a problem hiding this comment.
@kwlee2025cpp Thank you very much for careful work. Sorry for my slow pace of review. I only have feedback about warning messages and code comments, which can be handled in a future PR. I will merge now.
Regarding my earlier comment about #1234 (comment) having some unclear parts, I read through it again, and only 2 parts are unclear to me:
-
Is @beachdweller one of your previous GitHub accounts? The text "One more disclosure, a personal thread: in 2017..." implies this, but maybe not.
-
What is "robotics-grader" in "I would have cited Fortran-free robotics-grader containers (RoboRacer / F1TENTH on ROS 2), but 0.7.0's wheels just narrowed that to a niche."?
There is nothing wrong with having multiple GitHub accounts. I only asked because I was confused about authorship.
| warnings.warn( | ||
| f"E is ill-conditioned (cond(E) = {condE:.2g}); the generalized " | ||
| "Lyapunov solution may have reduced accuracy. The problem itself " | ||
| "is ill-conditioned for such E, so method='slycot' is not " |
There was a problem hiding this comment.
A warning like this has too much scope. The warning can only appear when method='scipy', but it has a claim about method='slycot'. If the underlying SLICOT method is improved and this warning message is no longer true, will we remember to update this warning? Probably not because it is only on method='scipy' code paths. I recommend that either the claim is removed (i.e., the warning does not refer to slycot) or a qualifying word is used, like "method='slycot' may not be more accurate" (so, "may" instead of "is").
| raise ControlArgument( | ||
| "method='scipy' requires E to be nonsingular; " | ||
| "a truly singular E (descriptor system) is not " | ||
| "supported by either method") |
There was a problem hiding this comment.
My comment about _warn_ill_conditioned_E() applies to this warning as well. Furthermore, "either method" requires that there are only 2 methods. In the future, there may be more than scipy and slycot methods, at which time this warning message text would need to change, whether or not a method='scipy' code path is affected.
| "is (almost) equal to 1; the discrete-time " | ||
| "Sylvester equation is singular") | ||
| Y = np.empty((n, m), dtype=complex) | ||
| TaY = np.empty((n, m), dtype=complex) # running Ta @ Y |
There was a problem hiding this comment.
What does #running Ta @ Y indicate? If it is only a reminder that TaY = np.empty() is to allocate the space before filling it in the loop, then delete the comment because it does not help with reading the code. This purpose is obvious, and a similar call is made for Y but no such comment is there.
|
Both answered, briefly:
Thank you for the review and for merging. The feedback on warning messages and code comments is welcome — happy to prepare that as a small follow-up whenever suits you. |
Summary
lyapanddlyapcurrently raiseControlArgument("method='scipy' not valid for ...")for three problems, so solving them requires the optionalslycotpackage:A X Eᵀ + E X Aᵀ + Q = 0,A X Aᵀ − E X Eᵀ + Q = 0,A X Qᵀ − X + C = 0.This PR implements pure scipy/numpy fallbacks for all three, so they work
when
slycotis not installed. Whenslycotis installed it remains thedefault (
method='slycot'), and the new tests cross-validate the scipyresults against it.
What it adds
Generalized Lyapunov (continuous and discrete). The equation is reduced
to a standard Lyapunov equation by a congruence transform with
inv(E)(
à = E⁻¹A,Q̃ = E⁻¹Q E⁻ᵀ, leavingXunchanged), then solved withscipy.linalg.solve_{continuous,discrete}_lyapunov. This requiresEto benonsingular. SLICOT
SG03AD(method='slycot'), based on the generalizedSchur method of Penzl, additionally handles singular
E; that case raises aclear
ControlArgumentpointing the user tomethod='slycot'.Discrete-time Sylvester. Solved by the Bartels–Stewart method: complex
Schur factorizations of
AandQᵀ, then a column-by-column backsubstitution. This matches the
O(n³ + m³)complexity of theHessenberg–Schur algorithm in SLICOT
SB04QD(method='slycot').Singularity (a pair of eigenvalues with
λᵢ·λⱼ ≈ 1) is detected and raisesa clear
ControlArgument.Ill-conditioned
E. Because the generalized-Lyapunov fallback invertsE, accuracy degrades whenEis poorly conditioned. AUserWarningisnow issued when
cond(E) > 1/√eps, recommendingmethod='slycot'. (Thisalso surfaces a case in the continuous path that was previously silently
inaccurate.)
Fixes included
slycotguards: theexcept ImportErrorbranches bound the sentinels under the wrong names(
sb0qmdforsb04qd,sb04adforsg03ad), so on a slycot-lessinstall the intended
Nonefallbacks were never set under the names thecode references.
dlyapdocstring note (implied a Kronecker-product method);corrected to Bartels–Stewart, with Golub–Nash–Van Loan and
Bartels–Stewart references added to the
Notes/References.Testing
test_lyap_g,test_dlyap_g, andtest_dlyap_sylvester(scipy result matches slycot to~1e-9…1e-16).test_lyap_g_ill_conditioned_E_scipy(parametrized overlyap/dlyap)asserts the ill-conditioned-
EUserWarning.test_dlyap_sylvester_singular_scipyasserts the singular-pencilControlArgument.19 passed, 12 skipped(no slycot) /31 passed(slycot).Only
control/mateqn.pyandcontrol/tests/mateqn_test.pyare touched.Scope
This is an intentionally small, self-contained slice. It is the first of a
short series factoring a larger "scipy fallbacks for slycot-gated routines"
effort into independently reviewable PRs; the matrix-equation solvers go
first because they are foundational and easy to validate against SLICOT.
gramis the planned next slice.Alternatives considered
The same
inv(E)reduction is used by pyMOR's dense scipy Lyapunovbackend. Factoring the pencil
(A, E)withscipy.linalg.qzinstead ofinverting
E(asharolddoes) is more robust for ill-conditionedEand is a natural follow-up. Truly singular
E(descriptor systems withinfinite generalized eigenvalues) is out of scope here and continues to
require
method='slycot'; a pure-scipy path for that case would needspectral-projector deflation of the infinite spectrum.