-
Notifications
You must be signed in to change notification settings - Fork 438
Fix deprecation and formatting warnings #187
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
Fix deprecation and formatting warnings #187
Conversation
control/freqplot.py
Outdated
# Create a unique label to fix bug in matplotlib<=2.1 | ||
# See https://github.com/matplotlib/matplotlib/issues/9024 | ||
import random | ||
figlabel = str(random.randint(1, 1e6)) |
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.
why not plt.clf() instead?
also, this is in a loop over systems (starting line 139) -- as I understand, this creates a new axis every time (unclear to me if the old one remains in existence). Why I try this:
>>> g = control.tf(1,[1,1])
>>> h = control.tf(1,[1,2])
>>> _ = control.bode_plot([g, h])
>>> plt.show()
I see only the frequency response of h.
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.
plt.clf(): the default behavior for matplotlib
is that if you call plot twice, you get the new data on top of the previous data. bode
is supposed to emulate that => this PR implements that behavior.
For the example with a list of TFs: this is the incorrect behavior, but need to look into it. Could be that we need to pass a tuple? (Can't check right now.)
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.
If I run this:
#!/usr/bin/env python3
import matplotlib.pyplot as plt
import control
g = control.tf(1,[1,1])
h = 1 - g
control.bode_plot(g)
control.bode_plot(h)
plt.show()
on your branch, I see only h.
My understanding from the docs is that different labels will result in different axes being created; from that link:
The axes label attribute has been exposed for this purpose: if you want two axes that are otherwise identical to be added to the figure, make sure you give them unique labels
I didn't realize you were aiming to emulate Matplotlib's behaviour; this is reasonable, but seems like it will require some way to check if there are existing Bode plot axes. If Matplotlib has the equivalent of Matlab's "findobj", it should be doable: search the current figure for axes with labels "pycontrol-bode-mag", and "[...]-phase", and re-use them if they exist; if they don't, create them.
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 seems to work:
diff --git a/control/freqplot.py b/control/freqplot.py
index c53e83f..368bdfc 100644
--- a/control/freqplot.py
+++ b/control/freqplot.py
@@ -135,6 +135,20 @@ def bode_plot(syslist, omega=None, dB=None, Hz=None, deg=None,
else:
omega = sp.logspace(np.log10(omega_limits[0]), np.log10(omega_limits[1]), endpoint=True)
+ if Plot:
+ fig = plt.gcf()
+ ax_mag = None
+ ax_phase = None
+ for ax in fig.axes:
+ if ax.get_label() == 'pycontrol-bode-mag':
+ ax_mag = ax
+ elif ax.get_label() == 'pycontrol-bode-phs':
+ ax_phase = ax
+ if ax_mag is None or ax_phase is None:
+ plt.clf()
+ ax_mag = plt.subplot(211, label = 'pycontrol-bode-mag')
+ ax_phase = plt.subplot(212, label = 'pycontrol-bode-phs', sharex=ax_mag)
+
mags, phases, omegas, nyquistfrqs = [], [], [], []
for sys in syslist:
if (sys.inputs > 1 or sys.outputs > 1):
@@ -172,7 +186,6 @@ def bode_plot(syslist, omega=None, dB=None, Hz=None, deg=None,
if (Plot):
# Magnitude plot
- ax_mag = plt.subplot(211);
if dB:
pltline = ax_mag.semilogx(omega_plot, 20 * np.log10(mag), *args, **kwargs)
else:
@@ -186,7 +199,6 @@ def bode_plot(syslist, omega=None, dB=None, Hz=None, deg=None,
ax_mag.set_ylabel("Magnitude (dB)" if dB else "Magnitude")
# Phase plot
- ax_phase = plt.subplot(212, sharex=ax_mag);
if deg:
phase_plot = phase * 180. / math.pi
else:
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 extended the above to the gang-of-four plots too: https://github.com/roryyorke/python-control/tree/rory/mplib-ax-warnings-2 ; this covers all of the subplot() calls in control/. Successive calls to bode_plot
and gangof4_plot
will add to the existing axes, as desired.
However, this breaks examples/pvtol-nested.py, specifically, figure 6, which uses subplot(211)
and subplot(212)
to add annotations to the gain and phase plots. This example will eventually break when Matplotlib changes subplot() as warned in the deprecation we're looking at. (When running that example, Matplotlib warns that matplotlib.pyplot.hold() is going to disappear at some point too.)
I think to keep python-control working with the new Matplotlib way, we'll have to overhaul the plotting a bit; we should allow the user to get hold of the line objects (for creating legends), and to the axes (for adding lines, points, text, ...). All of this is quite a bit of work, so perhaps we should leave it to after 0.8.0.
Separately, it might be good to add run the examples as part of Travis CI; and perhaps we should turn deprecation warnings into errors in all Travis CI runs.
Sub-axes are created with specific axis labels, which are strings; the Bode plot uses 'pycontrol-bode-{mag,phs}', and the gang-of-four plot uses 'pycontrol-gof-{t,s,ps,cs}' to identify its plots. These axes are then used, if possible; if not, the figure is cleared and the set of axes created.
…ke/python-control into roryyorke-rory/mplib-ax-warnings-2
I've grabbed the changes above. I think I am going to change the labels a bit and will also add a fix in For the labels, I'm thinking that the format should be |
Changed the labels for subplots to be of the form `control-plotname-subfigname` for consistency. Moved around some of the code from @roryyorke to put it in a single place. Added documentation.
The matplotlib hold commands are deprecated, so they have been removed. Also updated the code for augmenting bode plots to use the new axes labels that are part of the matplotlib 2.1 modifications.
I've put in a unit test to make sure the right number of lines are showing up on plots (crude, but would have caught the error that @roryyorke noted). I also updated the axes label names, put in some documentation (code comments + user manual), and fixed the This should now be good to go, but would like someone else to have a look. |
Looks good to me. I asked on Matplotlib-users about how we're using the labels [1], and the response was it's fine. However, the recommendation was to not rely on the pyplot state machine (gcf, subplot, etc.). I'll give this some thought, and open a new issue to discuss it. [1] https://mail.python.org/pipermail/matplotlib-users/2018-January/001255.html |
This PR fixes the warnings identified in issues #182 and #183:
Changed
\dot
and\cdot
to\\dot
and\\cdot
to avoid warnings in docstrings that contain mathematics (thesphinx
documentation identifies this as the preferred method for including mathemathics).Added a randomly generated label to
subplot
calls to avoid a warning message in matplotlib 2.1.