-
Notifications
You must be signed in to change notification settings - Fork 438
Frequency plot improvements #1011
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
Frequency plot improvements #1011
Conversation
4b3597f
to
ffee41f
Compare
fig.suptitle(title, **kwargs) | ||
|
||
elif frame == 'axes': | ||
# TODO: move common plotting params to 'ctrlplot' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This same TODO comment is already freqplot.py, and it is not clear what the comment means here (in ctrlplot). Should the TODO comment be deleted here in favor of the one in freqplot.py ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading it again, I think this TODO comment is to be able to
rcParams = config._get_param('ctrlplot', 'rcParams', rcParams)
If so, then I understand the motivation, and OK to keep both TODO comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got another PR coming that will address both TODOs. Doing things in stages so that everything is in (slightly) smaller chunks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. I am almost done reviewing this one.
Excellent improvements! I did not find any technical errors, but here are a few misprints in examples/cds110_bode-nyquist.ipynb:
OK to leave it as-is, but one of the commit messages has a misprint: "reectoring/regularization of ax keyword processing" should have "refactoring". After you consider/apply the above, it is ready to merge. |
29f3713
to
343df2c
Compare
This PR adds a number of improvements for frequency plots (Bode, Nyquist, etc). The changes are motivated by my experience in using python-control for a course, where I found a few things that were harder to do than they should have been.
Changes in this PR:
Allow
label
keyword in frequency response commands to override default label generation.Fix bug in processing
indent_radius
keyword whennyquist_plot
is passed a system (keyword was not getting sent through tonyquist_response
).Restore functionality that allows omega to be specified as a list of 2 elements (indicating a range) in all frequency response/plotting routines. This used to work for
nyquist
but got removed at some point. It now works for all frequency response commands.Fix up the
ax
keyword processing to allow arrays or lists + uniform processing in all freqplot routines.Fix up
rcParam
s processing (uniformity + remove unnecessary code). (This needs more work in a future pass to eliminate the rcParams keywords that are part of frequency plotting functions and to pull separate time and frequency plot parameters into a single place. There are some TODOs in the code marking what needs to be done.)Added new suptitle() function to add titles that are better centered (on axes instead of figure). Using this for frequency domain plots for now; will update for time domain plots as well.
Set up
frd
as factory function with keywords, including setting the signal/system names. Calls toct.FRD
should how be replaces withct.frd
, though the former will still work.Allow Bode and Nyquist plots for FRD systems with different omega vectors (Fix nyquist plotting from FrequencyResponseData #996) as well as mixtures of FRD and other LTI systems.
Added a notebook
examples/cds110-bode_nyquist.ipynb
that shows a lot of the new functionality in the context of frequency domain analysis.Minor: changed the way that
freq_label
(frequency axis label) is implemented so that it can be more easily overriden (use fstring with '{units}' instead of '%' => can put in custom units).For the most part these changes just make it easier to do things that were already possible. Example of some new code that makes use of these features (from
examples/cds110-bode_nyquist.ipynb
):For the mixed FRD functionality (motivated by comments I made in #996), here's an example showing how Nyquist plots can be combined in ways that weren't possible before: