Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
murrayrm merged 21 commits intopython-control:masterfromgonmolina:unsup_rlocus_gain_selection
Jan 5, 2018
Merged

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

murrayrm merged 21 commits intopython-control:masterfromgonmolina:unsup_rlocus_gain_selection
Jan 5, 2018

Conversation

gonmolina
Copy link
Contributor

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
Copy link

Coverage Status

Coverage increased (+0.4%) to 77.778% when pulling8b829cc on gonmolina:unsup_rlocus_gain_selection into6ada795 on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 77.778% when pulling6d55d57 on gonmolina:unsup_rlocus_gain_selection into6ada795 on python-control:master.

@coveralls
Copy link

coveralls commentedMar 31, 2017•
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 76.955% when pullingebd3904 on gonmolina:unsup_rlocus_gain_selection into6ada795 on python-control:master.

@slivingstonslivingston self-assigned thisMar 31, 2017
@coveralls
Copy link

coveralls commentedApr 2, 2017•
edited
Loading

Coverage Status

Coverage decreased (-0.5%) to 76.822% when pulling5438746 on gonmolina:unsup_rlocus_gain_selection into6ada795 on python-control:master.

@slivingston
Copy link
Member

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

@slivingston
Copy link
Member

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

@coveralls
Copy link

coveralls commentedJun 13, 2017•
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 76.68% when pulling6c813ce on gonmolina:unsup_rlocus_gain_selection into6ada795 on python-control:master.

@coveralls
Copy link

coveralls commentedJun 13, 2017•
edited
Loading

Coverage Status

Coverage decreased (-0.9%) to 76.428% when pulling6e4d14f on gonmolina:unsup_rlocus_gain_selection into6ada795 on python-control:master.

@gonmolina
Copy link
ContributorAuthor

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
Copy link
Member

@gonmolina thanks working on it now.

Copy link
Member

@slivingstonslivingston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Besides usingand 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 like6e4d14f 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.

open_loop_poles = den.roots
open_loop_zeros = num.roots

if (open_loop_zeros.size != 0) & (open_loop_zeros.size < open_loop_poles.size):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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



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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 givenin PEP 8. E.g., after I edit the comment text a little,

iflen(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
Copy link
Member

@gonmolina thanks. The changes are good.

@slivingston
Copy link
Member

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

@gonmolina
Copy link
ContributorAuthor

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

@slivingston
Copy link
Member

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
Copy link
ContributorAuthor

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
Copy link

coveralls commentedJul 1, 2017•
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 76.741% when pulling4eedde4 on gonmolina:unsup_rlocus_gain_selection into6ada795 on python-control:master.

@slivingston
Copy link
Member

@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 thatyou suggest above?

@gonmolina
Copy link
ContributorAuthor

yes, you can do it.

@murrayrm
Copy link
Member

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

@murrayrmmurrayrm added this to the0.8.0 milestoneDec 27, 2017
@murrayrm
Copy link
Member

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

@murrayrmmurrayrm dismissedslivingston’sstale reviewJanuary 5, 2018 05:31

Changes have been implemented.

@murrayrmmurrayrm merged commit92ccf2d intopython-control:masterJan 5, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@slivingstonslivingstonslivingston left review comments

Assignees

@slivingstonslivingston

Labels
None yet
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

4 participants
@gonmolina@coveralls@slivingston@murrayrm
[8]ページ先頭
©2009-2025 Movatter.jp