Implement getpoles for a vector K passed as a parameter

[mzaffalon]

The current implementation ignore the request to compute the roots at specific values of K, instead relying on OrdinaryDiffEq to set the values of K.

[mzaffalon]

getpoles(G; K) with K a number uses OrdinaryDiffEq.jl to compute the roots at values of k determined by the adaptive algorithm of OrdinaryDiffEq.jl. The implementation is however wrong in that roots[i] corresponds to K[i+1]. I don't know how to fix this. I also have too little experience with control systems to say whether it would make sense to compute the roots only at specific values of K and too little experience with OrdinaryDiffEq to use a different method to obtain the correct values of K.

EDIT: Maybe https://github.com/JuliaControl/ControlSystems.jl/blob/master/src/root_locus.jl#L40 should be replaced with push!(poleout,integrator.k[end]).

[codecov]

Codecov Report

Merging #775 (d811599) into master (a726079) will increase coverage by 95.52%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #775       +/-   ##
===========================================
+ Coverage    0.00%   95.52%   +95.52%     
===========================================
  Files           3        4        +1     
  Lines         292      313       +21     
===========================================
+ Hits            0      299      +299     
+ Misses        292       14      -278     

Impacted Files	Coverage Δ
src/root_locus.jl	97.72% <100.00%> (+97.72%)	⬆️
src/ControlSystems.jl	100.00% <0.00%> (ø)
src/simulators.jl	87.50% <0.00%> (+87.50%)	⬆️
src/timeresp.jl	94.90% <0.00%> (+94.90%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mzaffalon]

It is not clear to me how to test the second test without plotting it.

[baggepinnen]

Hello and thanks for this PR :)

It is not clear to me how to test the second test without plotting it.

I'm not sure what you mean with the second test?

[mzaffalon]

I'm not sure what you mean with the second test?

Sorry, badly formulated. I meant this test: https://github.com/JuliaControl/ControlSystems.jl/blob/master/test/test_rootlocus.jl#L5-L9

[baggepinnen]

Ah, yeah I didn't know of a good way to test that either. The problem it exposed was that some of the root loci switched at some point, which led to me introducing the hungarian algorithm to prevent it. I think it's okay if we leave it like that, just make sure you have run the plot and it still looks nice and that's probably good enough :)

[mzaffalon]

Can one test plots in CI?

[baggepinnen]

yes, but it's very cumbersome. One can test that a plot "looks similar" to a saved plot, but unfortunately, most plotting packages make small changes to the visual appearance of plots between versions such that most naive metrics for plot similarity fail, even though the plot is still correct. We do have a plot test bot that displays a number of test plots in PRs automatically, not sure why that didn't run here. I'll try to activate it so you can see how it works.

@JuliaControlBot test-plots

[JuliaControlBot]

This is an automated message.
Plots were compared to references. 11/11 images have changed, see differences below.
After pulling this PR, please update the reference images by creating a PR to ControlExamplePlots.jl here.

Difference	Reference Image	New Image
❌ 0.048
✔️ 0.011
✔️ 0.001
✔️ 0.001
✔️ 0.002
✔️ 0.001
✔️ 0.0
❌ 0.034
❌ 0.06
✔️ 0.0
✔️ 0.012

[mzaffalon]

Thank you for the explanation, @baggepinnen. Indeed the first plot has both cosmetic changes but also the legend is incorrect.

I pushed a commit that re-enables the plot and more importantly, also the roots for k=0 in the method with adaptive k.

Perhaps unrelated: would you consider having a (const) struct RootLocus with the root locus, so that one can do plot(rt::RootLocus; kargs)? It would depart from the MATLAB interface but would be more Julian.

[baggepinnen]

Indeed the first plot has both cosmetic changes but also the legend is incorrect.

Actually, these changes are by choice. While the left plot looks better in this simple case, the code to make it look that way was very fragile when several systems were plotted in the same figure, and different plot backends interpreted latex in different ways. We thus decided to include less custom logic to style the plots in favor of a better worst-case result and less maintenance burden since the logic to keep the plot looking nice had to be updated quite often.

would you consider having a (const) struct RootLocus with the root locus, so that one can do plot(rt::RootLocus; kargs)? It would depart from the MATLAB interface but would be more Julian.

That sounds nice :) We could add this struct as long as we do not break the current interface. Maybe the old plot could be implemented in terms of a plot recipe on the struct.

[mzaffalon]

We could add this struct as long as we do not break the current interface. Maybe the old plot could be implemented in terms of a plot recipe on the struct.

OK, I will give it a try. I am not familiar with Plots.jl so I may need some hints.

What does it take to merge this PR with the changes to getpoles? Plotting the struct directly seems unrelated to this PR.

[baggepinnen]

The SimResult struct that is returned from lsim, ste etc. has plot recipes defined, the same machinery would be useful for a RootLocus struct.

https://github.com/JuliaControl/ControlSystems.jl/blob/master/lib/ControlSystemsBase/src/plotting.jl#L105