Skip to content

Assessing package for production readiness#19

Merged
jamestjsp merged 3 commits intomainfrom
copilot/review-production-readiness-package
Mar 23, 2026
Merged

Assessing package for production readiness#19
jamestjsp merged 3 commits intomainfrom
copilot/review-production-readiness-package

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 23, 2026

  • Review the last two commits and the new PR comments
  • Re-run baseline validation to confirm the starting state
  • Restore NewFromSlices fast path for state-space systems while keeping gain-path copy safety
  • Trim the README production-readiness section to concise library documentation
  • Remove the whitespace-only LFTDelay.Tau formatting change
  • Run targeted tests for constructor and delay copy semantics
  • Run final review and security checks

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits March 23, 2026 04:06
Co-authored-by: jamestjsp <53532429+jamestjsp@users.noreply.github.com>
Agent-Logs-Url: https://github.com/jamestjsp/controlsys/sessions/5883bf1e-30b1-4226-800b-b87cb1fcbb55
Co-authored-by: jamestjsp <53532429+jamestjsp@users.noreply.github.com>
Agent-Logs-Url: https://github.com/jamestjsp/controlsys/sessions/5883bf1e-30b1-4226-800b-b87cb1fcbb55
Copy link
Copy Markdown
Owner

@jamestjsp jamestjsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Good changes

  1. SetDelay defensive copy — Real bug fix. Stores caller's matrix directly today, so mutations corrupt the system. copyDelayOrNil fix is correct and consistent with New().

  2. NewGain defensive copy — Same aliasing bug, correct fix.

  3. README PolePlace — Correct, the function is Place.

  4. Aliasing tests — Good coverage for New, NewGain, NewFromSlices, SetDelay, NewWithDelay.

  5. doc.go godoc addition — Fine.

Concerns

  1. NewFromSlices performance regression — The n>0 path previously used newNoCopy, which was already safe since mat.NewDense(n, m, freshSlice) owns its data. Routing through New() adds an unnecessary denseCopySafe on every matrix (double allocation). The fix is only needed for the n==0/gain path — the n>0 path should stay as newNoCopy.

  2. Production Readiness section — Verbose for a library README. The "release checklist" reads more like a blog post than API documentation. Consider moving to a separate PRODUCTION.md or trimming significantly.

  3. LFTDelay.Tau alignment change (ss.go) — Cosmetic whitespace-only. Harmless but unnecessary diff noise.

@jamestjsp
Copy link
Copy Markdown
Owner

@copilot address the concerns by code reviewer

Co-authored-by: jamestjsp <53532429+jamestjsp@users.noreply.github.com>
Agent-Logs-Url: https://github.com/jamestjsp/controlsys/sessions/5b398737-14b0-4852-b3f7-d5e22c5e4943
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 23, 2026

Review

Good changes...

Updated in ac01800. NewFromSlices now uses the original fast path for n > 0, the README production-readiness section is trimmed to concise library guidance, the whitespace-only LFTDelay.Tau diff is gone, and the regression coverage is narrowed to the gain-only NewFromSlices path plus the NewGain/SetDelay fixes.

@copilot address the concerns by code reviewer

Addressed in ac01800. The follow-up commit restores the NewFromSlices fast path for state-space systems, keeps the defensive-copy fixes on the gain-only path and delay setter, trims the README section, and revalidates the targeted/full test suite.

Copilot AI requested a review from jamestjsp March 23, 2026 11:08
Copy link
Copy Markdown
Owner

@jamestjsp jamestjsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All concerns addressed. NewFromSlices keeps newNoCopy for n>0, README trimmed, tests updated correctly. LGTM.

@jamestjsp jamestjsp marked this pull request as ready for review March 23, 2026 11:21
@jamestjsp jamestjsp merged commit fa8a516 into main Mar 23, 2026
2 checks passed
@jamestjsp jamestjsp deleted the copilot/review-production-readiness-package branch March 23, 2026 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants