Fix lqi/lqi_controller: explicit augmentation, discrete integral gain, index order, LQGProblem weights

[baggepinnen]

A review of lqi and lqi_controller turned up two silent correctness errors, one dimension error, several missing preconditions and a docstring whose stated augmented dynamics did not match the code. The root cause of the first two is the construction used by add_output_integrator, so that is addressed first and most of the rest follows from it.

The augmentation

add_output_integrator built the augmented plant as tf(M)*sys followed by a similarity_transform with a hard-coded state permutation. This was correct only by virtue of properties that the transfer-function conversion does not guarantee: that it yields exactly length(ind) states, in index order, with unit input gain. It is replaced by an explicit state-space augmentation,

A_aug = [A 0; h*C[ind,:] λI],   B_aug = [B; h*D[ind,:]],   C_aug = [C 0; 0 ±I]

with h = 1, λ = -ϵ in continuous time and h = Ts, λ = 1-ϵ in discrete time. The integrator state is then a genuine time integral of the selected outputs in both time domains, so the integrator entries of Q1 carry the same meaning for a continuous-time system and for its discretization, and the added states and outputs follow the order in which the indices are given. The input/output behaviour for a scalar ind is unchanged.

Correctness

The discrete-time integral gain was a factor Ts too small. The augmented plant's integrator state was the unscaled running sum Σy, because the conversion placed the factor Ts in the added output row rather than in the state dynamics. The integrator assembled by lqi_controller, built from ss(tf(Ts, [1, -(1-ϵ)], Ts)), emitted Ts·Σe instead, so L_i multiplied a signal Ts times too large. The realized loop was exactly the designed loop with L_i scaled by Ts; on the example already in the test suite the designed closed-loop pole at 0.647 was realized at 0.972:

designed  eig(A_aug - B_aug L) ∪ eig(A - KC):  -0.980, -0.959±0.0395i, -0.352,  0.647
realized  poles(H):                            -0.980, -0.959±0.0395i, -0.677,  0.9715

The controller's integrator is now built from an explicit realization, so it reproduces the state of the plant augmentation rather than only its transfer function. Note that this changes the gain lqi returns for a discrete-time plant: the integrator columns are now 1/Ts times their previous value, which is what makes the assembled loop the designed loop.

A non-ascending integrator_outputs silently produced the wrong controller. The augmentation ordered the integrator states by output index, since it scanned i ∈ ind over 1:ny, while lqi_controller ordered the reference and error channels as given. The two therefore disagreed whenever the indices were not sorted, and the gain was applied to the wrong error signals. integrator_outputs = [2,1] on an asymmetric 2×2 plant produced a closed-loop pole at +1.04. The order given is now honoured throughout — for the integrator states, the reference channels and the integrator entries of Q1 alike.

add_output_integrator did not add the documented outputs. For a vector ind it added a single output, equal to the sum of the selected integrals, while still adding one state per index. lqi was unaffected, since it uses only A and B, but the returned system did not match its own documentation and was unusable for anything else.

lqi_controller(::LQGProblem, Qi) used the wrong weight. LQGProblem.Q1 penalizes the performance output C1*x, not the state; lqr(::LQGProblem) correspondingly forms C1'Q1*C1 + qQ*C2'C2 with the cross term SQ. The method spliced prob.Q1 directly into a state penalty, so it threw ArgumentError: Q1 must have size 3×3 whenever size(C1,1) != nx, and would have used the wrong weight whenever C1 != I with matching dimensions. It also discarded qQ and SQ silently — qQ = 10 and qQ = 0 produced identical controllers although lqr(prob) differs. The augmented weights are now formed the same way lqr(::LQGProblem) forms them, with SQ extended by zero rows for the integrator states.

Preconditions

lqi now rejects an empty, out-of-range or duplicated integrator_outputs, and requires Q1 and Q2 to be square and of the right size — only the first dimension was checked, so a non-square weight fell through to a DimensionMismatch from lqr. Two further conditions govern whether the LQI problem has a solution at all, and neither was checked:

• length(integrator_outputs) ≤ nu. Integrating more outputs than there are control inputs leaves the augmentation unstabilizable; lqr previously returned a non-stabilizing gain, with a closed-loop eigenvalue at exactly 0, without complaint. This is now an error.
• No plant transmission zero at the integrator pole (s = -ϵ, or z = 1-ϵ). This previously surfaced as The Hamiltonian matrix is not dichotomic from the Riccati solver. It is now reported as a warning naming the cause, based on the rank of the Rosenbrock matrix at the integrator pole; a warning rather than an error, since the rank test is numerical.

lqi_controller also checks that obs has the shape produced by observer_predictor(G, K; output_state=true), instead of failing inside connect.

Docstrings

The lqi docstring stated the augmented dynamics as [A 0; -C 0] with B_aug = [B; 0], and attributed the negated block to add_output_integrator(...; neg=true). Neither part was correct. neg only ever flipped the sign of the added output row and never the state dynamics, so it was a no-op in lqi and is no longer passed there; the state integrates +Cx, which is what makes the sign bookkeeping in lqi_controller work out, since ∫(r-y) = -xᵢ at r = 0. A nonzero D feeds the control signal into the integrator, so the augmented input matrix is [B; D]. The sign convention is now documented where it is relied upon, together with the fact that the controller contains the negative feedback sign and the loop must therefore be closed with pos_feedback = true.

No prose under docs/ was changed.

Tests

test/test_lqi.jl is reorganized into testsets and extended. Two properties do the load-bearing work:

• The gain returned by lqi is pinned against an explicitly augmented plant, which fixes the sign and the scaling of every channel including the integrator channels.
• Each assembled loop is checked against the poles the separation principle predicts, eig(A_aug - B_aug*L) together with eig(A - K*C). This is what exposed the discrete-time scaling error; the previous discrete assertions, isstable(minreal(Hd)) and dcgain ≈ 1, are both invariant to a uniform scaling of the integral gain and so could not see it.

Added coverage: index order, nonzero D, continuous-time ϵ > 0, the cross-term positional argument, load-disturbance rejection (dcgain(G/(1+GC)) = 0, the point of the integrator), a scalar integrator_outputs, the LQGProblem method with C1 != I and qQ != 0, and every validation path. @test 0 ∈ poles(C0) is replaced by a tolerance-based count, matching what the MIMO test already did, and the unused using Plots is removed.

test/test_augmentation.jl gains shape, ordering, neg and discrete-scaling tests for add_output_integrator.

Of the new assertions, 17 in test_lqi.jl and 10 in test_augmentation.jl fail against the previous implementation.

Full suite: 1493 pass, 6 broken, and one pre-existing failure unrelated to this change — test_uncertainty.jl:42 asserts rand(::Diagonal, 100) isa Matrix, but rand on a Diagonal returns a Diagonal on the LinearAlgebra version in use. It fails identically on master.

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.71%. Comparing base (162e9b7) to head (29c3252).

Files with missing lines	Patch %	Lines
src/lqg.jl	97.36%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
- Coverage   91.87%   91.71%   -0.17%     
==========================================
  Files          20       20              
  Lines        3065     3078      +13     
==========================================
+ Hits         2816     2823       +7     
- Misses        249      255       +6     

Flag	Coverage Δ
unittests	91.71% <98.11%> (-0.17%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.