Repair control architecture and numerical contracts#163
Conversation
- untrack .ergo/plans.jsonl and ignore .ergo/ - explain order rejection when it would split a conjugate Schur pair - reject MaxRealPart thresholds that select no modes instead of falling back to the half-order heuristic - give GridTune candidates their own analysis-point map via withSampledController instead of a shallow model copy - reuse weight-response and product buffers across the frequency loop in frequencyGainRange (-30% allocs on the dynamic-weight path) - require strictly increasing passivity grids, matching tuning-goal grid validation, and document the FRDPassive grid contract Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14029acd0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for k, w := range omega { | ||
| s := e.sAt(w) | ||
| if err := e.evalStateSpaceInto(s, data[k*pm:(k+1)*pm]); err != nil { |
There was a problem hiding this comment.
For every frequency in a descriptor-model sweep, evalStateSpaceInto constructs a new ssEvalWorkspace, allocating several O(n²) complex buffers. Large frequency grids therefore generate substantial avoidable allocation and GC pressure, contrary to AGENTS.md's requirement to pre-allocate buffers outside loops and reuse LAPACK work arrays. Allocate one workspace before this loop and call evalFrSSInto with it for each frequency.
Useful? React with 👍 / 👎.
| if len(inputs) == 0 && len(outputsForPort) == 0 { | ||
| inputs = integerRange(inputCursor, port.Dimension) | ||
| outputsForPort = integerRange(outputCursor, port.Dimension) | ||
| inputCursor += port.Dimension | ||
| outputCursor += port.Dimension |
There was a problem hiding this comment.
When a component mixes explicit and implicit port bindings, the cursors advance only for implicit ports and do not account for channels already claimed explicitly. For example, an explicit first port bound to channel 0 followed by an implicit port is assigned channel 0 again and rejected as reused even when channel 1 is available; later implicit ports can similarly collide. Select the next unused input/output channels, or advance the cursors past explicit assignments.
Useful? React with 👍 / 👎.
1 participant
Summary
ModelArraystacking to preserve array rank and add explicit flattening concatenationv0.17.6-forkto the latest publishedv0.17.7-forkGridTune, withSystuneandLooptuneretained as compatibility wrappersWhy
Several APIs exposed shallow or ambiguous contracts: stacking silently flattened model grids, passivity sampling looked like certification, modal truncation depended on the current state basis, physical connections only combined metadata, and all named loop points and gain goals collapsed onto the same response. Those behaviors could produce plausible but mathematically incorrect results.
The revised modules own their invariants and workflows: model arrays own shape semantics, passivity results own their evidence status and grid, modal reduction owns Schur-block selection and projection, physical assembly owns topology and constraints, and tuning goals own point/response/grid/weight selection.
Compatibility and impact
Passiveremains as a deprecated compatibility alias forSampledPassive.ConcatModelArraysprovides the former flatten-and-append behavior explicitly.P*CconventionSystuneandLooptuneremain available and delegate toGridTuneC(sE-A)^{-1}B+DModalTruncatenow reports an explicit error when a requested order splits a complex-conjugate pair or aMaxRealPartthreshold selects no modes, instead of silently falling backValidation
go fix ./...go vet ./...go test -v -count=1 ./...go test -race -count=1 ./...go test -run '^$' -bench=. -benchmem -count=1 ./...(315.1 s, PASS)Focused tuning benchmarks show the true point-aware induced-norm checks increase runtime while reducing allocations; the Cartesian search now has an explicit
MaxEvaluationsbound.CI follow-up
The initial Linux/amd64 run exposed an architecture-dependent logarithmic-grid endpoint rounding difference. Commit
9040de6makes the generated passivity grid explicitly own its lower and Nyquist endpoints. Native arm64, Darwin amd64, full local race tests, both GitHub test jobs, and both downstream-consumer jobs now pass.