From cc67e4472eb21bedb3fad6cbd4ed6afe0be09b9a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Mar 2026 04:06:47 +0000 Subject: [PATCH 1/3] fix constructor aliasing and document readiness Co-authored-by: jamestjsp <53532429+jamestjsp@users.noreply.github.com> Agent-Logs-Url: https://github.com/jamestjsp/controlsys/sessions/5883bf1e-30b1-4226-800b-b87cb1fcbb55 --- README.md | 31 +++++++++++++++++++-- delay.go | 4 +-- delay_test.go | 34 +++++++++++++++++++++++ doc.go | 5 ++++ ss.go | 12 ++++----- ss_test.go | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 122aee6..067c189 100644 --- a/README.md +++ b/README.md @@ -10,11 +10,38 @@ Supports continuous and discrete LTI models with MIMO capability. go get github.com/jamestjsp/controlsys ``` -> **Note:** This package depends on a [gonum fork](https://github.com/jamestjsp/gonum) for additional LAPACK routines. Add this to your `go.mod`: +> **Note:** This package depends on a [gonum fork](https://github.com/jamestjsp/gonum) for additional LAPACK routines. Because `replace` directives do not propagate to downstream modules, applications that import `controlsys` must add this to their own `go.mod`: > ``` > replace gonum.org/v1/gonum => github.com/jamestjsp/gonum v0.17.2-fork > ``` +## Production Readiness + +This package is suitable for production use when you need numerically robust LTI modeling and analysis in Go, but it is still a numerical library rather than a turn-key control platform. In practice that means the main production questions are dependency management, numerical validation, and API discipline rather than service-style concerns like logging or network hardening. + +### What is already in good shape + +- **Input validation:** constructors and most mutating APIs reject dimension mismatches, invalid sample times, negative delays, mixed delay models, and unsupported control-design inputs. +- **Defensive copies at API boundaries:** constructors and delay/name setters copy caller-owned matrices and slices so a system is not accidentally corrupted by later mutations to the original inputs. +- **Verification coverage:** the repository includes focused unit tests, cross-validation tests, MATLAB/MathWorks reference checks, examples, benchmarks, `go vet`, and CI race-test coverage. +- **Operational simplicity:** the dependency surface is intentionally small and the package is pure Go aside from LAPACK-backed numerical routines provided by gonum. + +### Known production caveats + +- **Forked gonum dependency:** consumers must intentionally pin the gonum fork in their own module. That should be treated as a supply-chain decision and reviewed the same way you would review any vendored numerical runtime. +- **Numerical sensitivity is model-dependent:** ill-conditioned realizations, nearly uncontrollable or nearly unobservable systems, and repeated or clustered poles can produce results that are mathematically valid but sensitive to perturbations. Validate critical models against trusted references. +- **Transport delays are specialized:** continuous delays often require Pade approximation, while discrete fractional delays rely on Thiran-style approximations. Choose the delay model deliberately and verify frequency-domain behavior for the operating range you care about. +- **Mutable systems are not implicitly synchronized:** analysis methods are safe for read-only use, but setters mutate the receiver in place. If a model is shared across goroutines, treat it as immutable or clone it first with `Copy`. +- **Versioning discipline matters:** if you deploy this in a control or estimation stack, pin an explicit module version and rerun your plant- and controller-specific validation on upgrades. + +### Recommended release checklist for downstream users + +1. Pin both `controlsys` and the gonum fork to explicit versions. +2. Run `go vet ./...` and `go test -v -count=1 -race ./...` in your consuming application. +3. Add model-specific regression tests using non-symmetric `A` matrices so transposition bugs cannot hide behind diagonal fixtures. +4. Benchmark the exact operations that sit on your control loop or planning path with `go test -bench=. -benchmem`. +5. Compare poles, zeros, margins, and representative time responses against an external reference for every mission-critical plant. + ## Features - **Three representations:** state-space, transfer function, zero-pole-gain (ZPK) with bidirectional conversion @@ -107,7 +134,7 @@ func main() { | `Dlqr` | Discrete-time LQR regulator | | `Lqe` | Kalman filter (observer) gain | | `Lqi` | LQR with integral action | -| `Pole` | Pole placement | +| `Place` | Pole placement | | `Care` | Continuous algebraic Riccati equation | | `Dare` | Discrete algebraic Riccati equation | diff --git a/delay.go b/delay.go index 6248f60..b13bc64 100644 --- a/delay.go +++ b/delay.go @@ -26,7 +26,7 @@ func (sys *System) SetDelay(delay *mat.Dense) error { if err := validateDelay(delay, p, m, sys.Dt); err != nil { return err } - sys.Delay = delay + sys.Delay = copyDelayOrNil(delay) return nil } @@ -1643,7 +1643,7 @@ func (sys *System) GetDelayModel() (H *System, tau []float64) { // Pull all delays into LFT lft, err := sys.PullDelaysToLFT() if err != nil { - // If PullDelaysToLFT fails (e.g. non-decomposable residual), + // If PullDelaysToLFT fails (e.g. non-decomposable residual), // we fallback to just extracting InternalDelay if it exists, // or return the original system. if sys.LFT == nil { diff --git a/delay_test.go b/delay_test.go index 21256a9..ba52559 100644 --- a/delay_test.go +++ b/delay_test.go @@ -29,6 +29,26 @@ func TestNewWithDelay(t *testing.T) { } } +func TestNewWithDelayCopiesDelayMatrix(t *testing.T) { + delay := mat.NewDense(1, 1, []float64{3}) + sys, err := NewWithDelay( + mat.NewDense(1, 1, []float64{0.5}), + mat.NewDense(1, 1, []float64{1}), + mat.NewDense(1, 1, []float64{1}), + mat.NewDense(1, 1, []float64{0}), + delay, + 1.0, + ) + if err != nil { + t.Fatal(err) + } + + delay.Set(0, 0, 99) + if got := sys.Delay.At(0, 0); got != 3 { + t.Fatalf("delay alias detected: got %v, want 3", got) + } +} + func TestNewWithDelayNil(t *testing.T) { A := mat.NewDense(1, 1, []float64{0.5}) B := mat.NewDense(1, 1, []float64{1}) @@ -62,6 +82,20 @@ func TestSetDelayNegative(t *testing.T) { } } +func TestSetDelayCopiesInputMatrix(t *testing.T) { + sys, _ := NewFromSlices(1, 1, 1, + []float64{0.5}, []float64{1}, []float64{1}, []float64{0}, 1.0) + delay := mat.NewDense(1, 1, []float64{2}) + if err := sys.SetDelay(delay); err != nil { + t.Fatal(err) + } + + delay.Set(0, 0, 99) + if got := sys.Delay.At(0, 0); got != 2 { + t.Fatalf("delay alias detected: got %v, want 2", got) + } +} + func TestSetDelayFractionalDiscrete(t *testing.T) { sys, _ := NewFromSlices(1, 1, 1, []float64{0.5}, []float64{1}, []float64{1}, []float64{0}, 1.0) diff --git a/doc.go b/doc.go index bc4a037..7a03c02 100644 --- a/doc.go +++ b/doc.go @@ -3,4 +3,9 @@ // transport delays, state-space and transfer-function representations, // frequency response analysis, discretization, simulation, and // model reduction. +// +// Constructors defensively copy caller-owned matrices and slices so a +// System can be treated as a stable value after creation. Methods that +// configure delays, names, or notes mutate the receiver, so shared +// systems should be copied with Copy before concurrent mutation. package controlsys diff --git a/ss.go b/ss.go index c60960a..6c2adf8 100644 --- a/ss.go +++ b/ss.go @@ -10,7 +10,7 @@ import ( // LFTDelay holds the internal delay representation using a linear // fractional transformation (LFT) structure. type LFTDelay struct { - Tau []float64 + Tau []float64 B2, C2, D12, D21, D22 *mat.Dense } @@ -189,11 +189,12 @@ func NewGain(D *mat.Dense, dt float64) (*System, error) { if D == nil { return nil, fmt.Errorf("D matrix required for gain system: %w", ErrDimensionMismatch) } + p, m := D.Dims() return &System{ A: &mat.Dense{}, B: &mat.Dense{}, C: &mat.Dense{}, - D: D, + D: denseCopySafe(D, p, m), Dt: dt, }, nil } @@ -241,13 +242,10 @@ func NewFromSlices(n, m, p int, a, b, c, d []float64, dt float64) (*System, erro } else { Dm = &mat.Dense{} } - return &System{ - A: &mat.Dense{}, B: &mat.Dense{}, C: &mat.Dense{}, - D: Dm, Dt: dt, - }, nil + return NewGain(Dm, dt) } - return newNoCopy(A, B, C, D, dt) + return New(A, B, C, D, dt) } func (sys *System) Copy() *System { diff --git a/ss_test.go b/ss_test.go index 5627196..9e21ba1 100644 --- a/ss_test.go +++ b/ss_test.go @@ -98,6 +98,36 @@ func TestNewGain(t *testing.T) { } } +func TestNewCopiesInputMatrices(t *testing.T) { + A := mat.NewDense(2, 2, []float64{0, 1, -2, -3}) + B := mat.NewDense(2, 1, []float64{0, 1}) + C := mat.NewDense(1, 2, []float64{1, 0}) + D := mat.NewDense(1, 1, []float64{0}) + + sys, err := New(A, B, C, D, 0) + if err != nil { + t.Fatal(err) + } + + A.Set(0, 0, 99) + B.Set(0, 0, 99) + C.Set(0, 0, 99) + D.Set(0, 0, 99) + + if got := sys.A.At(0, 0); got != 0 { + t.Fatalf("A alias detected: got %v, want 0", got) + } + if got := sys.B.At(0, 0); got != 0 { + t.Fatalf("B alias detected: got %v, want 0", got) + } + if got := sys.C.At(0, 0); got != 1 { + t.Fatalf("C alias detected: got %v, want 1", got) + } + if got := sys.D.At(0, 0); got != 0 { + t.Fatalf("D alias detected: got %v, want 0", got) + } +} + func TestNewFromSlices(t *testing.T) { sys, err := NewFromSlices(2, 1, 1, []float64{0, 1, -2, -3}, @@ -126,6 +156,50 @@ func TestNewFromSlicesGain(t *testing.T) { } } +func TestNewFromSlicesCopiesInputSlices(t *testing.T) { + a := []float64{0, 1, -2, -3} + b := []float64{0, 1} + c := []float64{1, 0} + d := []float64{0} + + sys, err := NewFromSlices(2, 1, 1, a, b, c, d, 0) + if err != nil { + t.Fatal(err) + } + + a[0] = 99 + b[0] = 99 + c[0] = 99 + d[0] = 99 + + if got := sys.A.At(0, 0); got != 0 { + t.Fatalf("A alias detected: got %v, want 0", got) + } + if got := sys.B.At(0, 0); got != 0 { + t.Fatalf("B alias detected: got %v, want 0", got) + } + if got := sys.C.At(0, 0); got != 1 { + t.Fatalf("C alias detected: got %v, want 1", got) + } + if got := sys.D.At(0, 0); got != 0 { + t.Fatalf("D alias detected: got %v, want 0", got) + } +} + +func TestNewGainCopiesInputMatrix(t *testing.T) { + D := mat.NewDense(1, 2, []float64{3, 4}) + + sys, err := NewGain(D, 0) + if err != nil { + t.Fatal(err) + } + + D.Set(0, 0, 99) + if got := sys.D.At(0, 0); got != 3 { + t.Fatalf("D alias detected: got %v, want 3", got) + } +} + func TestCopy(t *testing.T) { sys, _ := NewFromSlices(2, 1, 1, []float64{0, 1, -2, -3}, From cd8253517357faf535505adc75ef16c22f145ddc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Mar 2026 04:07:25 +0000 Subject: [PATCH 2/3] polish production readiness wording Co-authored-by: jamestjsp <53532429+jamestjsp@users.noreply.github.com> Agent-Logs-Url: https://github.com/jamestjsp/controlsys/sessions/5883bf1e-30b1-4226-800b-b87cb1fcbb55 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 067c189..f66f9e5 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ go get github.com/jamestjsp/controlsys ## Production Readiness -This package is suitable for production use when you need numerically robust LTI modeling and analysis in Go, but it is still a numerical library rather than a turn-key control platform. In practice that means the main production questions are dependency management, numerical validation, and API discipline rather than service-style concerns like logging or network hardening. +This package is suitable for production use when you need numerically robust LTI modeling and analysis in Go. However, it is still a numerical library rather than a turn-key control platform. In practice that means the main production questions are dependency management, numerical validation, and API discipline rather than service-style concerns like logging or network hardening. ### What is already in good shape From ac01800639cb5b05c2564b147d8c4e3cbb9c344c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Mar 2026 11:05:44 +0000 Subject: [PATCH 3/3] address reviewer follow-ups Co-authored-by: jamestjsp <53532429+jamestjsp@users.noreply.github.com> Agent-Logs-Url: https://github.com/jamestjsp/controlsys/sessions/5b398737-14b0-4852-b3f7-d5e22c5e4943 --- README.md | 28 +++++----------------------- doc.go | 7 +++---- ss.go | 2 +- ss_test.go | 36 ++++++++++-------------------------- 4 files changed, 19 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index f66f9e5..d87215c 100644 --- a/README.md +++ b/README.md @@ -17,30 +17,12 @@ go get github.com/jamestjsp/controlsys ## Production Readiness -This package is suitable for production use when you need numerically robust LTI modeling and analysis in Go. However, it is still a numerical library rather than a turn-key control platform. In practice that means the main production questions are dependency management, numerical validation, and API discipline rather than service-style concerns like logging or network hardening. +This package is intended to be usable in production control and estimation code, with the usual caveat that numerical software still needs application-specific validation. -### What is already in good shape - -- **Input validation:** constructors and most mutating APIs reject dimension mismatches, invalid sample times, negative delays, mixed delay models, and unsupported control-design inputs. -- **Defensive copies at API boundaries:** constructors and delay/name setters copy caller-owned matrices and slices so a system is not accidentally corrupted by later mutations to the original inputs. -- **Verification coverage:** the repository includes focused unit tests, cross-validation tests, MATLAB/MathWorks reference checks, examples, benchmarks, `go vet`, and CI race-test coverage. -- **Operational simplicity:** the dependency surface is intentionally small and the package is pure Go aside from LAPACK-backed numerical routines provided by gonum. - -### Known production caveats - -- **Forked gonum dependency:** consumers must intentionally pin the gonum fork in their own module. That should be treated as a supply-chain decision and reviewed the same way you would review any vendored numerical runtime. -- **Numerical sensitivity is model-dependent:** ill-conditioned realizations, nearly uncontrollable or nearly unobservable systems, and repeated or clustered poles can produce results that are mathematically valid but sensitive to perturbations. Validate critical models against trusted references. -- **Transport delays are specialized:** continuous delays often require Pade approximation, while discrete fractional delays rely on Thiran-style approximations. Choose the delay model deliberately and verify frequency-domain behavior for the operating range you care about. -- **Mutable systems are not implicitly synchronized:** analysis methods are safe for read-only use, but setters mutate the receiver in place. If a model is shared across goroutines, treat it as immutable or clone it first with `Copy`. -- **Versioning discipline matters:** if you deploy this in a control or estimation stack, pin an explicit module version and rerun your plant- and controller-specific validation on upgrades. - -### Recommended release checklist for downstream users - -1. Pin both `controlsys` and the gonum fork to explicit versions. -2. Run `go vet ./...` and `go test -v -count=1 -race ./...` in your consuming application. -3. Add model-specific regression tests using non-symmetric `A` matrices so transposition bugs cannot hide behind diagonal fixtures. -4. Benchmark the exact operations that sit on your control loop or planning path with `go test -bench=. -benchmem`. -5. Compare poles, zeros, margins, and representative time responses against an external reference for every mission-critical plant. +- Pin both `controlsys` and the required gonum fork to explicit versions. +- Validate mission-critical models against an external reference, especially for ill-conditioned realizations and delay-heavy systems. +- `System` values are mutable. Use `Copy` before sharing a model across goroutines that may mutate names, delays, notes, or other receiver state. +- The repository CI runs `go vet ./...` and `go test -v -count=1 -race ./...`; those are the recommended baseline checks for downstream integrations. ## Features diff --git a/doc.go b/doc.go index 7a03c02..0ee6d24 100644 --- a/doc.go +++ b/doc.go @@ -4,8 +4,7 @@ // frequency response analysis, discretization, simulation, and // model reduction. // -// Constructors defensively copy caller-owned matrices and slices so a -// System can be treated as a stable value after creation. Methods that -// configure delays, names, or notes mutate the receiver, so shared -// systems should be copied with Copy before concurrent mutation. +// Methods that configure delays, names, or notes mutate the receiver, +// so shared systems should be copied with Copy before concurrent +// mutation. package controlsys diff --git a/ss.go b/ss.go index 6c2adf8..d956636 100644 --- a/ss.go +++ b/ss.go @@ -245,7 +245,7 @@ func NewFromSlices(n, m, p int, a, b, c, d []float64, dt float64) (*System, erro return NewGain(Dm, dt) } - return New(A, B, C, D, dt) + return newNoCopy(A, B, C, D, dt) } func (sys *System) Copy() *System { diff --git a/ss_test.go b/ss_test.go index 9e21ba1..61aefa3 100644 --- a/ss_test.go +++ b/ss_test.go @@ -156,45 +156,29 @@ func TestNewFromSlicesGain(t *testing.T) { } } -func TestNewFromSlicesCopiesInputSlices(t *testing.T) { - a := []float64{0, 1, -2, -3} - b := []float64{0, 1} - c := []float64{1, 0} - d := []float64{0} +func TestNewGainCopiesInputMatrix(t *testing.T) { + D := mat.NewDense(1, 2, []float64{3, 4}) - sys, err := NewFromSlices(2, 1, 1, a, b, c, d, 0) + sys, err := NewGain(D, 0) if err != nil { t.Fatal(err) } - a[0] = 99 - b[0] = 99 - c[0] = 99 - d[0] = 99 - - if got := sys.A.At(0, 0); got != 0 { - t.Fatalf("A alias detected: got %v, want 0", got) - } - if got := sys.B.At(0, 0); got != 0 { - t.Fatalf("B alias detected: got %v, want 0", got) - } - if got := sys.C.At(0, 0); got != 1 { - t.Fatalf("C alias detected: got %v, want 1", got) - } - if got := sys.D.At(0, 0); got != 0 { - t.Fatalf("D alias detected: got %v, want 0", got) + D.Set(0, 0, 99) + if got := sys.D.At(0, 0); got != 3 { + t.Fatalf("D alias detected: got %v, want 3", got) } } -func TestNewGainCopiesInputMatrix(t *testing.T) { - D := mat.NewDense(1, 2, []float64{3, 4}) +func TestNewFromSlicesGainOnlyCopiesInputSlice(t *testing.T) { + d := []float64{3, 4} - sys, err := NewGain(D, 0) + sys, err := NewFromSlices(0, 2, 1, nil, nil, nil, d, 0) if err != nil { t.Fatal(err) } - D.Set(0, 0, 99) + d[0] = 99 if got := sys.D.At(0, 0); got != 3 { t.Fatalf("D alias detected: got %v, want 3", got) }