Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7d37f683e
ℹ️ 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".
| // Ideal derivative controllers may be improper on their own while their | ||
| // closed-loop characteristic is well-defined. Test the characteristic directly. | ||
| func pidTuningIdealStable(plant *System, c *PID2) bool { | ||
| transfer, err := plant.TransferFunction(nil) |
There was a problem hiding this comment.
Preserve hidden modes in ideal-derivative stability checks
When tuning an ideal PD/PID (Kd != 0 and Tf == 0), this path checks a transfer-function characteristic instead of the full state-space interconnection. TransferFunction eliminates uncontrollable or unobservable modes, so a nonminimal plant containing a hidden unstable mode can be accepted and reported with Stability == "closed-loop-poles", even though the corresponding feedback interconnection retains that unstable state. Check the original plant's internal modes or preserve the full closed-loop dynamics before certifying stability.
Useful? React with 👍 / 👎.
| if power > 0 { | ||
| result.ResidualLagOne = cross / power |
There was a problem hiding this comment.
Normalize the lag-one residual metric correctly
When residual energy changes over the validation interval, dividing the lag product only by the current-sample power does not produce a valid lag-one correlation or regression coefficient. For example, residuals beginning [10, 1, 0.1] yield a value near 10, despite a correlation being bounded by 1. Normalize by both lagged powers (and consistently center the sequences) so ResidualLagOne remains an interpretable diagnostic.
Useful? React with 👍 / 👎.
Summary
Process Lab's PID workspace needs controller families and formulas, direct frequency-response tuning, and plant estimation from arbitrary input/output records. Add these numerical APIs while preserving the existing
Pidtunebehavior.Validation and limits
go test ./...andgo vet ./...pass. Tests cover formula realizations, independent target/margin equations, direct FRD agreement, noise-free and noisy identification, initialization, bounds and cancellation.These are native numerical methods with explicit evidence, not bit-identical proprietary MATLAB algorithms. Finite-band FRD and exact-delay margins do not certify global stability. Process Lab consumes immutable revision
d7d37f683e40through a module pseudo-version.