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

gonmolina · 2017-02-16T22:29:00Z

…points in real axis.

…points of real axis.

slivingston · 2017-02-17T01:53:16Z

Is there any way to make your proposed features without breaking the public API?

As a matter of style, I agree with some of the changes, e.g., avoiding camelCase in root_locus, but I think that they are better done as part of a batch improvement of style that goes into a single version increment.

slivingston · 2017-02-17T02:02:42Z

I will wait until you comment before doing a thorough review. However, I skimmed some of your work in ee7c35d, and most of the refactoring is good. In terms of the API, tests are failing on root_locus and a call to _convertToTransferFunction.

Whether _convertToTransferFunction should have the prefix _ might be worth discussing, but that might be outside the scope of this PR.

slivingston · 2017-02-17T02:14:29Z

Regarding the comments near the top of rlocus.py where you list changes and the date, it is much better to place those notes about changes in your commit message because:

file history is studied through commit history;
changelogs in the files can become difficult to search compared to root CHANGELOG.

Some of the files have author, date, and change notes, but they are mostly vestigial and not consistently added. They might be deleted soon.

murrayrm · 2017-02-17T07:18:52Z

control/rlocus.py

    """Root locus plot

    Calculate the root locus by finding the roots of 1+k*TF(s)
    where TF is self.num(s)/self.den(s) and each k is an element
-    of kvect.
+    of gvect.


This should still be kvect, based on the call signature.

murrayrm · 2017-02-17T07:19:42Z

control/rlocus.py


    Parameters
    ----------
-    sys : LTI object
+    dinsys : LTI object


Is there a reason to use dinsys? Most (all?) other functions use sys for the system argument. What does dinsys stand for??

I had problems while I was testing the code (maybe an import sys in the worng place). I will rename it to sys and test again.

murrayrm · 2017-02-17T07:20:58Z

control/rlocus.py


-rlocus = root_locus
+rlocus = root_locus


File should end in a new line, no?

restore original API (variables Plot, PrintGain), and calling _convertToTransferFunction fix kvect word comment sys as variable name in input argument add the on line in the last line

gonmolina · 2017-02-17T20:14:54Z

@slivingston
I have taken into account all comments you post and I have made the corrections. I am not programmer so probably there are several mistakes related to collaborate in a project (like changing the API). This is because it is the first time I try to participate in this kind of work.
There are any guide to collaborate with this particular project ? There are some automatic check that to do before the pull request?
I have plot several root locus and lines look nice using default gains calculation.

@murrayrm I made suggested corrections.

Thanks a lot.

tolerance is modified if axis are given.

coveralls · 2017-02-18T21:27:58Z

Coverage increased (+0.7%) to 78.001% when pulling 32fe9b6 on gonmolina:smart-rlocus into 5ab74cf on python-control:master.

…eformed.

coveralls · 2017-02-19T03:28:07Z

Coverage increased (+0.7%) to 78.03% when pulling 03df169 on gonmolina:smart-rlocus into 5ab74cf on python-control:master.

slivingston · 2017-03-11T01:13:46Z

@gonmolina I will review your recent changes tomorrow.

Regarding some guide for collaboration on this project, there is not an explicit one. The general style guides for Python apply: PEP 8 and PEP 257. Many practices used for the NumPy project we also aspire to have here, so consider consulting the NumPy contributing guide. Some details, e.g., about NumPy project governance, are not relevant here. Another guide that is good to try to follow is that of TuLiP.

Regarding automatic checks on the pull request (PR), tests are performed using Travis CI. You can find results from jobs for PRs at https://travis-ci.org/python-control/python-control/pull_requests

slivingston · 2017-03-12T22:30:41Z

Several of the changes that I am requesting involve rebasing and modifying commits in this PR. You might already know how, but in case not, two useful beginning references are:

Alternatively, if you add commits such that the total diff of all combined commits in this PR is good, then I can just squash them into a single commit and merge that.

slivingston · 2017-03-12T22:40:16Z

Is the code in this PR ported from Octave ?

slivingston · 2017-03-12T22:45:56Z

I ask because Octave is under the GPL, and I do not want to GPL-pollute the python-control sourcetree.

E.g., compare with https://sourceforge.net/p/octave/control/ci/tip/tree/inst/rlocus.m

slivingston · 2017-03-12T22:14:27Z

control/rlocus.py

 # $Id$

 # Packages used by this module
+from functools import partial


Why did you move the import of partial to here? Similarly, why were the imports of scipy.signal and pylab moved?

Are you changing the code to follow the style that groups imports according to standard library, other packages, etc.? If so, can you move such style-only changes to a dedicated commit? History is easier to understand that way.

slivingston · 2017-03-12T22:25:32Z

control/rlocus.py

@@ -80,6 +88,7 @@ def root_locus(sys, kvect=None, xlim=None, ylim=None, plotstr='-', Plot=True,
    PrintGain: boolean (default = True)
        If True, report mouse clicks when close to the root-locus branches,
        calculate gain, damping and print
+    plotstr: string that declare of the rlocus (see matplotlib)


Can you add details to this description? E.g.,

format string for rendering the root loci. Interpretation of the string is defined by matplotlib.pyplot.plot.

slivingston · 2017-03-12T22:27:33Z

control/rlocus.py

        ax = pylab.axes()

-        # plot open loop poles
+        # Plot open loop poles


I might be missing something, but it is not clear why you changed these "plot" words to uppercase "Plot". Is it merely a style change? If so, can you place it in a separate commit and indicate that it is a trivial change and only affects comments?

slivingston · 2017-03-12T22:55:05Z

After you address my question about originality, can you provide a nontrivial demonstration of the changes? I played some with your _default_gains, but I would enjoy your recommendation about an example.

gonmolina · 2017-03-13T21:13:47Z

@slivingston
I have ported from Octave a part of this PR. I have also used some part of the code of evans.sci (siclab) in this PR. However, for simplicity, most of the pionts calculated would be the same that calculated in Octave.

I just fix some problems I found in Octave. In Octave rlocus paths doesn't finished in zeros (I add some extra points) and I change the way that sort the roots (I use the same that It was used in rlocus.py). Other differences are axis limits, and asymptotes.

About the license problems. I think right now it is not original, or it is hard to defend the originality. I didn't try to hide it (if you compare you will see that even the comments in part of the code are the same).

I have some simple ideas to improve the algorithm, adding points in different positions that could result in fewer calculated points. The same for the finishing and adding points criteria. However I don't know when the work begins to be original. I mean, the main idea would be the same: add points to the rlocus up to a criteria is satisfied (evans.sci and rlocus.m use the same finishing criteria, but the way they add points is quite different between one and another). My idea is to use the same way to add points but changing the creterias. Could be that considered original enough?
There is any rule to define when it is original or when is a copy?

slivingston · 2017-03-14T03:59:55Z

Thanks for your explanation. It might have been better if I asked you whether it is derivative work, since that is the crucial question for copyright. Surely there is some originality in the process of porting.

The general guide about whether something is a derivative work is whether you depended critically on referring to most or all details of the original. E.g., did you copy-and-paste and then make necessary changes? Or, did you have to repeatedly compare with the original Octave implementation, line-by-line?

The copyright applies to the materials, not the ideas. So, if you provide a new implementation of an existing algorithm, all without detailed copying or porting of another existing implementation, then there should be no doubt about your new implementation being new (not derived).

gonmolina · 2017-03-14T19:15:52Z

@slivingston
Ok. Thanks for the comments. In that way, I think this PR have to be rejected! I will rewrite the code taking into consideration all comments you made and I will make a new PR. Thank you very much.

slivingston · 2017-03-31T19:50:46Z

@gonmolina thanks. I will review your new PR soon.

slivingston · 2017-06-25T17:56:11Z

for ease of future reference, note that the replacement PR is #138

Add smart selection of gains in root locus plot. Calculation of break…

ee7c35d

…points of real axis.

slivingston self-assigned this Feb 17, 2017

murrayrm reviewed Feb 17, 2017

View reviewed changes

Took into account comments made in pull request #132:

1b2bd96

restore original API (variables Plot, PrintGain), and calling _convertToTransferFunction fix kvect word comment sys as variable name in input argument add the on line in the last line

gonmolina closed this Feb 17, 2017

gonmolina reopened this Feb 17, 2017

gonmolina added 3 commits February 18, 2017 17:37

gains selected inside_defualt_gains.

f9c4df2

tolerance is modified if axis are given.

fix bug using np.max

1474e82

fix error in evaluation on break points

32fe9b6

add zeros to vector of roots when calculation of axis limits it is pr…

03df169

…eformed.

slivingston requested changes Mar 12, 2017

View reviewed changes

gonmolina closed this Mar 14, 2017

gonmolina deleted the smart-rlocus branch March 21, 2017 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gonmolina commented Feb 16, 2017

slivingston commented Feb 17, 2017

slivingston commented Feb 17, 2017

slivingston commented Feb 17, 2017

murrayrm Feb 17, 2017 •

edited

Loading

gonmolina Feb 17, 2017

murrayrm Feb 17, 2017 •

edited

Loading

gonmolina Feb 17, 2017

murrayrm Feb 17, 2017

gonmolina Feb 17, 2017

gonmolina commented Feb 17, 2017

coveralls commented Feb 18, 2017 •

edited

Loading

coveralls commented Feb 19, 2017 •

edited

Loading

slivingston commented Mar 11, 2017

slivingston commented Mar 12, 2017

slivingston commented Mar 12, 2017

slivingston commented Mar 12, 2017

slivingston Mar 12, 2017

slivingston Mar 12, 2017

slivingston Mar 12, 2017

slivingston commented Mar 12, 2017

gonmolina commented Mar 13, 2017

slivingston commented Mar 14, 2017

gonmolina commented Mar 14, 2017

slivingston commented Mar 31, 2017

slivingston commented Jun 25, 2017

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

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

Conversation

gonmolina commented Feb 16, 2017

slivingston commented Feb 17, 2017

slivingston commented Feb 17, 2017

slivingston commented Feb 17, 2017

murrayrm Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

gonmolina Feb 17, 2017

Choose a reason for hiding this comment

murrayrm Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

gonmolina Feb 17, 2017

Choose a reason for hiding this comment

murrayrm Feb 17, 2017

Choose a reason for hiding this comment

gonmolina Feb 17, 2017

Choose a reason for hiding this comment

gonmolina commented Feb 17, 2017

coveralls commented Feb 18, 2017 • edited Loading

coveralls commented Feb 19, 2017 • edited Loading

slivingston commented Mar 11, 2017

slivingston commented Mar 12, 2017

slivingston commented Mar 12, 2017

slivingston commented Mar 12, 2017

slivingston Mar 12, 2017

Choose a reason for hiding this comment

slivingston Mar 12, 2017

Choose a reason for hiding this comment

slivingston Mar 12, 2017

Choose a reason for hiding this comment

slivingston commented Mar 12, 2017

gonmolina commented Mar 13, 2017

slivingston commented Mar 14, 2017

gonmolina commented Mar 14, 2017

slivingston commented Mar 31, 2017

slivingston commented Jun 25, 2017

murrayrm Feb 17, 2017 •

edited

Loading

murrayrm Feb 17, 2017 •

edited

Loading

coveralls commented Feb 18, 2017 •

edited

Loading

coveralls commented Feb 19, 2017 •

edited

Loading