GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Control plot refactoring for consistent functionality by murrayrm · Pull Request #1034 · python-control/python-control Conversation
|
| Key | Value |
|---|---|
| ‘axes.labelsize’ | ‘small’ |
| ‘axes.titlesize’ | ‘small’ |
| ‘figure.titlesize’ | ‘medium’ |
| ‘legend.fontsize’ | ‘x-small’ |
| ‘xtick.labelsize’ | ‘small’ |
| ‘ytick.labelsize’ | ‘small’ |
Only those values that should be changed from the default need to be specified in the rcParams keyword argument. To override the defaults for all control plots, update the ct.rcParams dictionary entries.
The default values for style parameters for control plots can be restored using :func:~control.reset_rcParams.
The title keyword can be used to override the automatic creation of the plot title. The default title is a string of the form " plot for " where is a list of the sys names contained in the plot (which can be updated if the plotting is called multiple times). Use title=False to suppress the title completely. The title can also be updated using the control.ControlPlot.set_plot_title method for the returned control plot object.
The plot title is only generated if ax is None.
The following code illustrates the use of some of these customization features::
P = ct.tf([0.02], [1, 0.1, 0.01]) # servomechanism
C1 = ct.tf([1, 1], [1, 0]) # unstable
L1 = P * C1
C2 = ct.tf([1, 0.05], [1, 0]) # stable
L2 = P * C2
plt.rcParams.update(ct.rcParams)
fig = plt.figure(figsize=[7, 4])
ax_mag = fig.add_subplot(2, 2, 1)
ax_phase = fig.add_subplot(2, 2, 3)
ax_nyquist = fig.add_subplot(1, 2, 2)
ct.bode_plot(
[L1, L2], ax=[ax_mag, ax_phase],
label=["$L_1$ (unstable)", "$L_2$ (unstable)"],
show_legend=False)
ax_mag.set_title("Bode plot for $L_1$, $L_2$")
ax_mag.tick_params(labelbottom=False)
fig.align_labels()
ct.nyquist_plot(L1, ax=ax_nyquist, label="$L_1$ (unstable)")
ct.nyquist_plot(
L2, ax=ax_nyquist, label="$L_2$ (stable)",
max_curve_magnitude=22, legend_loc='upper right')
ax_nyquist.set_title("Nyquist plot for $L_1$, $L_2$")
fig.suptitle("Loop analysis for servomechanism control design")
plt.tight_layout()
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
coverage: 94.694% (+0.04%) from 94.65% |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
9a407ac to
a9ffba5
Compare
|
I will review this today (Saturday, 3 Aug). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
73c8c80 to
d66f465
Compare
This is taking a bit longer; I am almost done and will submit one review (instead of many small incremental comments). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! The consistency here is a good improvement for user/developer experience.
I made several small comments at the relevant lines and below. Ready to merge after you consider these.
If there are ruff options in pyproject, ruff itself should be install-able as an optional dependency. E.g., I think it fits in the test category (with pytest), or we can make a new category like dev in the [project.optional-dependencies] section. Here are examples in other popular projects for comparison:
ruff options but no ruff dependency in pyproject.tomlMany of the first row subplots now show horizontal axis numbers, even when these are the same and aligned with the bottom row subplots. E.g., doc/timeplot-mimo_step-default.png. I think this style is good, too, but I want to note it because there is not a commit in this PR that explicitly describes this change (so, maybe change was not intended?).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| string (see :func:`~matplotlib.pyplot.legend`). | ||
| legend_loc : int or str, optional | ||
| Include a legend in the given location. Default is 'center right', | ||
| with no legend for a single response. Use False to supress legend. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| with no legend for a single response. Use False to supress legend. | |
| with no legend for a single response. Use False to suppress legend. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This spelling misprint occurs in several other locations of this PR, but I am only marking this one. (Easy to find with grep supress.)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| omega_num=omega_num).plot(**kwargs) | ||
| *args, omega=None, omega_limits=None, omega_num=None, | ||
| Hz=False, **kwargs): | ||
| """Plot the response of the "Gange of 4" transfer functions for a system. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| """Plot the response of the "Gange of 4" transfer functions for a system. | |
| """Plot the response of the "Gang of 4" transfer functions for a system. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| labels, but no legend is generated in the plot (this can also be | ||
| accomplished by setting ``legend_map`` to ``False``. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| labels, but no legend is generated in the plot (this can also be | |
| accomplished by setting ``legend_map`` to ``False``. | |
| labels, but no legend is generated in the plot. (This can also be | |
| accomplished by setting ``legend_map`` to ``False``.) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Or delete ( and make the comment a non-parenthetical sentence.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| the ``legend_loc`` keyword argument is set to a string or integer, it | ||
| will set the position of the legend as described in the | ||
| :func:`matplotlib.legend` documentation. Finally, ``legend_map`` can be | ||
| set to an` array that matches the shape of the subplots, with each item |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| set to an` array that matches the shape of the subplots, with each item | |
| set to an array that matches the shape of the subplots, with each item |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| The :func:`~control.bode_plot`, :func:`~control.time_response_plot`, | ||
| and selected other commands can also accept a matplotlib format | ||
| string (e.g., 'r--'). The format string must appear as a positional |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| string (e.g., 'r--'). The format string must appear as a positional | |
| string (e.g., ``'r--'``). The format string must appear as a positional |
Without the code delimiters, Sphinx renders -- as an em dash.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Label every nth frequency on the plot. If not specified, no labels | ||
| are generated. | ||
| legend_loc : int or str, optional | ||
| Include a legend in the given location. Default is 'center right', |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| Include a legend in the given location. Default is 'center right', | |
| Include a legend in the given location. Default is 'upper right', |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| label(s). If data is a list, strings should be specified for each | ||
| system. | ||
| legend_loc : int or str, optional | ||
| Include a legend in the given location. Default is 'center right', |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| Include a legend in the given location. Default is 'center right', | |
| Include a legend in the given location. Default is 'upper right', |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| use matplotlib.pyplot.gca().axis('auto') and then set the axis | ||
| limits to the desired values. | ||
|
|
||
| 2. Pole/zero plts that use the continuous time omega-damping grid do |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| 2. Pole/zero plts that use the continuous time omega-damping grid do | |
| 2. Pole/zero plots that use the continuous time omega-damping grid do |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 2. Pole/zero plts that use the continuous time omega-damping grid do | ||
| not work with the ``ax`` keyword argument, due to the way that axes | ||
| grids are implemented. The ``grid`` argument must be set to | ||
| ``False`` or `'empty'`` when using the ``ax`` keyword argument. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| ``False`` or `'empty'`` when using the ``ax`` keyword argument. | |
| ``False`` or ``'empty'`` when using the ``ax`` keyword argument. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| response : :class:`~control.FrequencyResponseData` | ||
| Frequency response with inputs 'r' and 'd' and outputs 'y', and 'u' | ||
| representing the 2x2 matrix of transfer functions in the Gang of 4. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The return type here is ControlPlot.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for the careful review, @slivingston. Will address all of these in the coming day or two and try to merge this weekend (two additional, fairly minor, PRs coming after that). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Good catch! This was not intentional and there is now an inconsistency between tickmark labels in MIMO Bode plots versus MIMO time response plots. Since this is already a very large PR, I'm going to issue a separate PI to make that more uniform (and controllable). Remaining changes have been implemented. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Something odd going on in the unit tests: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
3270392 to
9f143ec
Compare
The file originates in #1022 so may need to rebase or merge. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The CI test can be fixed by adding a |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
This error is not triggered on |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.