move root locus implementation to ControlSystemsBase #1013
Conversation
|
Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/actions/runs/16493009614?check_suite_focus=true for more details. |
There was a problem hiding this comment.
Pull Request Overview
This PR moves the root locus implementation from the main ControlSystems package to ControlSystemsBase to make it available without requiring the heavier dependencies of ControlSystems. The implementation also includes improvements to the adaptive step-size algorithm for better performance.
- Move root locus functionality (
rlocus,rlocusplot) from ControlSystems to ControlSystemsBase - Improve the root locus algorithm with adaptive step sizing and Hungarian assignment
- Update documentation to reflect that ControlSystemsBase now includes root locus
Reviewed Changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test/runtests_controlsystems.jl |
Remove root locus test suite from ControlSystems |
src/ControlSystems.jl |
Remove root locus exports and imports from main package |
lib/ControlSystemsBase/test/runtests.jl |
Add root locus tests to ControlSystemsBase test suite |
lib/ControlSystemsBase/src/root_locus.jl |
Implement improved root locus algorithm with adaptive stepping |
lib/ControlSystemsBase/src/ControlSystemsBase.jl |
Add root locus exports and Hungarian import |
lib/ControlSystemsBase/Project.toml |
Add Hungarian.jl dependency |
docs/src/man/introduction.md |
Update documentation to reflect root locus now in ControlSystemsBase |
docs/src/lib/plotting.md |
Add rlocusplot documentation |
| stepsize /= 2.0 | ||
| # Ensure stepsize doesn't become too small | ||
| if stepsize < 100*eps(T) | ||
| @warn "Step size became extremely small, potentially stuck. Breaking loop." |
There was a problem hiding this comment.
The warning message could be more helpful by including the current step size value and suggesting potential causes or solutions.
| @warn "Step size became extremely small, potentially stuck. Breaking loop." | |
| @warn "Step size became extremely small (stepsize = $stepsize). This may indicate numerical instability or poor system conditioning. Consider increasing `tol` or `initial_stepsize`. Breaking loop." |
Copilot uses AI. Check for mistakes.
| if cost > 2 * tol # Cost is too high, reject step and reduce stepsize | ||
| stepsize /= 2.0 | ||
| # Ensure stepsize doesn't become too small | ||
| if stepsize < 100*eps(T) |
There was a problem hiding this comment.
The magic number 100*eps(T) should be defined as a named constant to make the minimum step size threshold more explicit and maintainable.
| if stepsize < 100*eps(T) | |
| if stepsize < MIN_STEP_SIZE_THRESHOLD |
Copilot uses AI. Check for mistakes.
| end | ||
| newpoles .= temppoles | ||
| # Cap stepsize to prevent overshooting K significantly in a single step | ||
| stepsize = min(stepsize, K/10) |
There was a problem hiding this comment.
The magic number K/10 should be defined as a named constant to make the maximum step size ratio more explicit and configurable.
| stepsize = min(stepsize, K/10) | |
| stepsize = min(stepsize, MAX_STEP_RATIO * K) |
Copilot uses AI. Check for mistakes.
| end | ||
|
|
||
| # Adaptive step size logic | ||
| if cost > 2 * tol # Cost is too high, reject step and reduce stepsize |
There was a problem hiding this comment.
The magic number 2 * tol should be defined as a named constant to make the cost threshold multiplier more explicit and configurable.
| if cost > 2 * tol # Cost is too high, reject step and reduce stepsize | |
| if cost > COST_THRESHOLD_MULTIPLIER * tol # Cost is too high, reject step and reduce stepsize |
Copilot uses AI. Check for mistakes.
| k_scalar = next_k_scalar # Advance k_scalar | ||
|
|
||
| if cost < tol # Cost is too low, increase stepsize | ||
| stepsize *= 1.1 |
There was a problem hiding this comment.
The magic number 1.1 should be defined as a named constant to make the step size growth factor more explicit and configurable.
| stepsize *= 1.1 | |
| stepsize *= STEP_SIZE_GROWTH_FACTOR |
Copilot uses AI. Check for mistakes.
No description provided.