add unsupervised calculation on gains for root locus plot. #138

gonmolina · 2017-03-21T01:42:42Z

Add gains up to a tolerance is achived.
@slivingston I wait for reviews. I have a file with some examples that are an exersice taken from Franklyn book. Let me know if you need it to test the algorithm. Do I have to put in a particular place? Where? Thanks in advance.

coveralls · 2017-03-21T03:17:21Z

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

fix bug plotting zeros when only zeros in zero exist

coveralls · 2017-03-23T01:59:06Z

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

fix bug plotting zeros when only zeros in zero exist

coveralls · 2017-03-31T17:58:33Z

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

…o unsup_rlocus_gain_selection

…and wn constant lines. grids ok in most of the examples.

coveralls · 2017-04-02T18:37:22Z

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

slivingston · 2017-05-28T20:24:20Z

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

slivingston · 2017-05-28T20:26:04Z

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

…r of poles and zeros

better selection of sgrid zetas and omega

coveralls · 2017-06-13T21:47:35Z

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

fix a minor bug.

coveralls · 2017-06-13T22:09:01Z

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

gonmolina · 2017-06-13T22:38:01Z

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

slivingston · 2017-06-25T01:10:53Z

@gonmolina thanks working on it now.

slivingston

Besides using and instead of &, I made several minor requests about style. Otherwise, this is ready.

You do not need to do it now for this PR, but for the future, commits like 6e4d14f that accomplish more than one distinct task should be split into multiple commits. One reason to do so is simple reverting of changes later if we need to.

slivingston · 2017-06-25T02:05:12Z

control/rlocus.py

+    open_loop_poles = den.roots
+    open_loop_zeros = num.roots
+
+    if (open_loop_zeros.size != 0) & (open_loop_zeros.size < open_loop_poles.size):


and should be used instead of &. The same request applies to lines 198 and 369.

slivingston · 2017-06-25T18:03:07Z

control/rlocus.py

+
+
+def _k_max(num, den, real_break_points, k_break_points):
+    """" Calculation the maximum gain for the root locus shown in tne figure"""


misprint: "tne" should be "the". Also, the first word should some verb. E.g., "Calculate"

slivingston · 2017-06-25T18:09:11Z

control/rlocus.py

    roots = []
    for k in kvect:
        curpoly = denp + k * nump
        curroots = curpoly.r
+        if len(curroots) < denp.order:
+            curroots = np.insert(curroots, len(curroots), np.inf)  # if i have less poles than open loop is because  i


The comment here overflows into the next line. For ease of reading, it should be moved to the line before. This style for block comments is given in PEP 8. E.g., after I edit the comment text a little,

if len(curroots) < denp.order: # If I have fewer poles than open loop, it is because I # have one at infinity. curroots = np.insert(curroots, len(curroots), np.inf)

slivingston · 2017-07-01T00:26:26Z

@gonmolina thanks. The changes are good.

slivingston · 2017-07-01T00:35:22Z

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

gonmolina · 2017-07-01T00:41:08Z

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

slivingston · 2017-07-01T00:44:56Z

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?

gonmolina · 2017-07-01T03:07:34Z

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)

coveralls · 2017-07-01T03:16:01Z

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

slivingston · 2017-09-17T20:29:33Z

@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?

gonmolina · 2017-09-18T17:00:37Z

yes, you can do it.

murrayrm · 2017-12-27T16:37:29Z

@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.

murrayrm · 2018-01-03T06:29:35Z

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).

Changes have been implemented.

gonmolina added 4 commits March 20, 2017 22:22

add unsupervised calculation on gains for root locus plot.

ffc3041

fix bug concatenating zero dimensional array

2aada8b

fix bug concatenating zero dimensional when test in matlab_test.py

7130434

fix bug with array==0! problem test in matlab_test.py

8b829cc

gonmolina added 2 commits March 22, 2017 22:14

fix bug with axis limits with no asymptotes

d4cbc95

fix bug plotting zeros when only zeros in zero exist

fix bug calculating kmax

6d55d57

gonmolina added 5 commits March 26, 2017 15:36

add unsupervised calculation on gains for root locus plot.

01e33ea

fix bug with axis limits with no asymptotes

95d6908

fix bug plotting zeros when only zeros in zero exist

add sgrid to rlocus plot, to the last version of unsupervised gains.

1b20bd4

add origin to rlocus plot and x=0 line (zeta =0 grid line).

5e47d1f

add grid arguments in docstring.

ebd3904

slivingston self-assigned this Mar 31, 2017

gonmolina added 3 commits March 31, 2017 18:00

Merge remote-tracking branch 'origin/unsup_rlocus_gain_selection' int…

3fb2c86

…o unsup_rlocus_gain_selection

reorder code that calculate defaults damp coefficents constant lines …

de87bb0

…and wn constant lines. grids ok in most of the examples.

formatting code to make easier sgrid() function in future.

5438746

gonmolina added 2 commits June 8, 2017 18:11

trying that algorithm work with 0 degrees root locus with equal numbe…

1e356b8

…r of poles and zeros

adding some support for 0 degrees root locus.

6c813ce

better selection of sgrid zetas and omega

add book reference of root locus calculation.

6e4d14f

fix a minor bug.

gonmolina added 2 commits June 14, 2017 10:07

fix minor bug when fartest point is zero.

a680c6a

fix minor bugs when that solves problems in paricular cases

81b73a6

slivingston mentioned this pull request Jun 25, 2017

Add smart selection of gains for root locus plot. Calculation of break… #132

Closed

slivingston previously requested changes Jun 25, 2017

View reviewed changes

Fixed requested issues.

62ed2aa

fix build problem (numerical problem)

4eedde4

murrayrm added this to the 0.8.0 milestone Dec 27, 2017

murrayrm merged commit 92ccf2d into python-control:master Jan 5, 2018

murrayrm mentioned this pull request Jan 30, 2021

fix rlocus timeout due to inefficient _default_wn calculation #527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unsupervised calculation on gains for root locus plot. #138

add unsupervised calculation on gains for root locus plot. #138

gonmolina commented Mar 21, 2017

coveralls commented Mar 21, 2017

coveralls commented Mar 23, 2017

coveralls commented Mar 31, 2017 •

edited

Loading

coveralls commented Apr 2, 2017 •

edited

Loading

slivingston commented May 28, 2017

slivingston commented May 28, 2017

coveralls commented Jun 13, 2017 •

edited

Loading

coveralls commented Jun 13, 2017 •

edited

Loading

gonmolina commented Jun 13, 2017

slivingston commented Jun 25, 2017

slivingston left a comment

slivingston Jun 25, 2017

slivingston Jun 25, 2017

slivingston Jun 25, 2017

slivingston commented Jul 1, 2017

slivingston commented Jul 1, 2017

gonmolina commented Jul 1, 2017

slivingston commented Jul 1, 2017

gonmolina commented Jul 1, 2017

coveralls commented Jul 1, 2017 •

edited

Loading

slivingston commented Sep 17, 2017

gonmolina commented Sep 18, 2017

murrayrm commented Dec 27, 2017

murrayrm commented Jan 3, 2018



		def _k_max(num, den, real_break_points, k_break_points):
		"""" Calculation the maximum gain for the root locus shown in tne figure"""

add unsupervised calculation on gains for root locus plot. #138

add unsupervised calculation on gains for root locus plot. #138

Conversation

gonmolina commented Mar 21, 2017

coveralls commented Mar 21, 2017

coveralls commented Mar 23, 2017

coveralls commented Mar 31, 2017 • edited Loading

coveralls commented Apr 2, 2017 • edited Loading

slivingston commented May 28, 2017

slivingston commented May 28, 2017

coveralls commented Jun 13, 2017 • edited Loading

coveralls commented Jun 13, 2017 • edited Loading

gonmolina commented Jun 13, 2017

slivingston commented Jun 25, 2017

slivingston left a comment

Choose a reason for hiding this comment

slivingston Jun 25, 2017

Choose a reason for hiding this comment

slivingston Jun 25, 2017

Choose a reason for hiding this comment

slivingston Jun 25, 2017

Choose a reason for hiding this comment

slivingston commented Jul 1, 2017

slivingston commented Jul 1, 2017

gonmolina commented Jul 1, 2017

slivingston commented Jul 1, 2017

gonmolina commented Jul 1, 2017

coveralls commented Jul 1, 2017 • edited Loading

slivingston commented Sep 17, 2017

gonmolina commented Sep 18, 2017

murrayrm commented Dec 27, 2017

murrayrm commented Jan 3, 2018

coveralls commented Mar 31, 2017 •

edited

Loading

coveralls commented Apr 2, 2017 •

edited

Loading

coveralls commented Jun 13, 2017 •

edited

Loading

coveralls commented Jun 13, 2017 •

edited

Loading

coveralls commented Jul 1, 2017 •

edited

Loading