Skip to content

fix blank bode plot in rootlocus_pid_designer #883

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 23, 2023

Conversation

sawyerbfuller
Copy link
Contributor

Starts with a small gain rather than zero to insure that the frequency response plots appear. Addresses #686

@sawyerbfuller sawyerbfuller linked an issue Apr 14, 2023 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Apr 14, 2023

Coverage Status

Coverage: 94.649% (+0.1%) from 94.535% when pulling fabcb37 on sawyerbfuller:rlpd_blank into 1e1c8eb on python-control:main.

@murrayrm
Copy link
Member

I tried running this on the example I pointed out in #686 and when I click on the root locus, nothing changes. I then tried the same system in sisotool and that also didn't work. I'm assuming this works for others? Any clues about what I am doing wrong?

Also, why is 0.001 the right initial_gain to use? We have a default value for this (sisotool.initial_gain) that is set to 1. Shouldn't we use that instead?

@murrayrm
Copy link
Member

Also, in looking at the code a bit, I am confused about the fact that the initial gain seems to be set using the Kp0, Ki0, and Kd0 parameters, but then it is reset to 0.001 for the gain that you choose to vary. Should you be able to override this through one of those parameters?

@sawyerbfuller
Copy link
Contributor Author

Regarding default gain: for rootlocus-pid-designer, in principle you are already starting with baseline nonzero kd0, kp0, and ki0, and the step responses show result of that. You are interested in any addition delta-k to one of those, so it makes sense to start that one with a small value. But a zero value erases Bose plot. Np.eps is another option but it would make a very low gain bode plot so I picked a kind of arbitrary small nonzero value.

@sawyerbfuller
Copy link
Contributor Author

Re non-interactive plot: doesn’t work in Jupiter, have to use magic command %matplotlib qt. Assuming you tried that, can you zoom? The way it works you have to click near one of its pre calculated points. It computes more if you zoom in.

@sawyerbfuller
Copy link
Contributor Author

@murrayrm On further thought based on your questions, I realized that a deltaK parameter would be useful needed for non-interactive use, so that the bode plot was usable. Added it in.

@murrayrm
Copy link
Member

Using %matplotlib fixed the issues I was having, so I added a bit of documentation.

@murrayrm murrayrm merged commit c06a205 into python-control:main Apr 23, 2023
@murrayrm murrayrm added this to the 0.9.4 milestone Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rootlocus_pid_designer plot looks strange + needs documentation
3 participants