move root locus implementation to ControlSystemsBase#1013
move root locus implementation to ControlSystemsBase#1013baggepinnen merged 9 commits intomasterfrom
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." |
| 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 |
| 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) |
| 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 |
| 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 |
2 participants
No description provided.