GeezerVison

Coverage increased (+0.4%) to 77.778% when pulling 8b829cc on gonmolina:unsup_rlocus_gain_selection into 6ada795 on python-control:master.

Coverage increased (+0.4%) to 77.778% when pulling 6d55d57 on gonmolina:unsup_rlocus_gain_selection into 6ada795 on python-control:master.

Coverage decreased (-0.4%) to 76.955% when pulling ebd3904 on gonmolina:unsup_rlocus_gain_selection into 6ada795 on python-control:master.

Coverage decreased (-0.5%) to 76.822% when pulling 5438746 on gonmolina:unsup_rlocus_gain_selection into 6ada795 on python-control:master.

@gonmolina can you add a citation in the docstring to the book that you used?

I am sorry for my delay; I lost track of my GitHub notifications feed. I anticipate finishing reviewing this within 2 days.

Coverage decreased (-0.6%) to 76.68% when pulling 6c813ce on gonmolina:unsup_rlocus_gain_selection into 6ada795 on python-control:master.

Coverage decreased (-0.9%) to 76.428% when pulling 6e4d14f on gonmolina:unsup_rlocus_gain_selection into 6ada795 on python-control:master.

I added a book as a reference.
I added possibility to draw a grid on s-plane similar to matlab (argument grid = True).
I have tested a lot this function and works fine.
I have wrote a script with several tests with a lot of cases and root locus figures seem to be fine. If you think this script can help you to test the algorithm I can add it to the repository (please tell me where is the right place to add it).
I have added support of 0 degrees root locus (positive feedback or k<0) in the algorithm. This works fine in all cases that I have tested, but I couldn't solve when the num.order=den.order. To solve this problem a complete new solution have to be written, mainly when lines are plotted and when roots are sorted. I raise an error when a root locus of this kind is demanded.

@slivingston I wait for your review.
Thanks in advance.
Gonzalo

@gonmolina thanks working on it now.

@gonmolina thanks. The changes are good.

Actually considering the failures on CI testing, there may have been a regression. I am trying to reproduce locally.

Is there any way to do the test locally in my machine without doing a push?
I think I can fix it....

There are several new changes outside the scope of the previous review, so I will review these when you tell me that they are ready.

Several of the commit messages mention fixing of bugs. If these existed before this PR, can you add regression tests for them, i.e., tests that demonstrate the bugs?

I made 2 changes:

• I added line 268 to ensure kmax> 0 when all the singular points are in 0. Problems getting kvect when deal with systems of the form 1/s^n. I do not think this is the problem while building.
• I greatly increased the gain after the kmax in line 209 so that the lines of the roots locus reach the zeros (in the figure) for all cases. I think this can be the problem since several groups of equal roots turn out to be about the same for too large gains. Solution: fewer points after kmax, smaller maximum gain, and separate them logarithmically. This problem was clear when I was trying to solve 0 degrees root locus with for systems with equal number of zeros and poles.

A regression test for the first problem is:

import control as ctrl
sys1=ctrl.tf([1],[1,0,0])
r,k = ctrl.rlocus(sys1, grid=True)

Coverage decreased (-0.6%) to 76.741% when pulling 4eedde4 on gonmolina:unsup_rlocus_gain_selection into 6ada795 on python-control:master.

@gonmolina it looks like you are permitting me to push changes to the branch associated with this pull request. Can I do so, to add the regression test that you suggest above?

yes, you can do it.

@slivingston I'm trying to clean up some of the PRs over the holiday break. Let me know if you are still working on this one.

I've run Travis CI on a local branch and all tests are OK: results.

@gonmolina, @slivingston Are we ready to merge or is more testing required? Seems like this has to be better than what was there before (placeholder).