Repository navigation
sgrid() and zgrid() call matplotlib.pyplot.figure() #291
Description
Activity
I'm wondering whether functions should be calling
plt.figure()at all. Shouldn't that be left to the highest-level function or even the user? Likewise withplt.show(). I think it’s better to leave the user with full control of where the plot is going and to allow additional formatting/additions.How about we have an optional keyword argument,
axsay, that is the matplotlib axis that the sub-function should add the plot to? IfaxisNonethen the function could create a new figure.If there are other ways to do this please suggest them. I can have a go at implementing this with
pzmapand also fixexamples/pvtol-nested.py.This is how Pandas plot functions work I think. See
axargument in documentation below for example:hist_series(self, by=None, ax=None, grid=True, xlabelsize=None, xrot=None, ylabelsize=None, yrot=None, figsize=None, bins=10, **kwds) Draw histogram of the input series using matplotlib. Parameters ---------- by : object, optional If passed, then used to form histograms for separate groups ax : matplotlib axis object If not passed, uses gca() grid : boolean, default True Whether to show axis grid lines xlabelsize : int, default None If specified changes the x-axis label size xrot : float, default None rotation of x axis labels ...Help on function gca in module matplotlib.pyplot: gca(**kwargs) Get the current :class:`~matplotlib.axes.Axes` instance on the current figure matching the given keyword args, or create one.Should we follow this approach?
I agree that we should not be calling
plt.figure()orplt.show()from within thepython-controlpackage, with the possible exception of inside thematlabsubpackage (to generate behavior that is consistent with how MATLAB operates).This fix might be a bit more complicated because some functions (like
bode_plotandgangof4_plot) generate subfigures and use labels to make sure that if you call the function twice in a row then you get a second set of lines on top of the first. So whatever you do as a fix should (somehow) work with those functions as well.It will get a bit tricky with functions that make plots with multiple axes but Pandas handles this with a
_subplotshelper function. The way I would handle subsequent updating or adding to plots is for these functions to return the axis (or array of axes) that were created. This also allows the user to go in and do custom formatting etc on subplots.Right now, how about I just implement this concept for a one-axis plot like
pzmap?Is now a good time to make a branch or shall I wait for the merge of your matrix/array PR?
I'm working on merging a bunch of pending PRs right now. This takes a while since I need to let Travis CI run for each one to make sure there are no interactions in the unit tests that cause it to fail. Should be done in another 30 min or so.
Also, down the road we could make some of these plot functions into object methods. Like this:
H = control.tf([2, 5, 1], [1, 2, 3]) H.pzmap()I'm working on this now (sorry for the delay...).
I've solved the problem for
zgrid(). But just want to check in with you @murrayrm that I understand the intended use cases correctly and get your advice.As I have it now, the following example (borrowed from MATLAB documentation) works as follows:
>>> H = tf([2 -3.4 1.5],[1 -1.6 0.8],-1) >>> r, k = rlocus(H) >>> zgrid() >>> plt.show()In this case, a new figure is generated very similar to MATLAB when plt.show() is called. The second use case is as follows:
>>> fig = plt.figure(1) >>> r, k = rlocus(H) >>> zgrid() >>> plt.show()In this case, rlocus and zgrid add things to the current axis of the existing figure.
A third option is something like this:
>>> fig, axes = plt.subplots(2, 1) >>> r, k = rlocus(H1, ax=axes[1]) >>> r, k = rlocus(H2, ax=axes[2]) >>> for ax in axes: >>> zgrid(ax=ax) # Later we might want to add ax.zgrid() >>> plt.show()Is this the functionality we want? Are these good examples of use cases?
Note: This is a slight departure from MATLAB here because functions like rlocus and pzmap rename the window name of the figure in MATLAB (e.g. to "Root Locus"). Here, I will only change the axis title to "Root Locus" instead, leaving the window title alone.
I just noticed some other functionality that this is going to conflict with. The current rlocus function gets a list of all existing figures and makes the name of the new figure
"Root Locus " + str(rloc_num)whererloc_numis an incrementing integer. E.g."Root Locus 5". Not sure whether we want to keep this behaviour if we are moving towards targeting individual axes rather than requiring separate figures for each plot.So, I think I have a working version. Except one test failed:
FAILED (failures=1, skipped=117) Test failed: <unittest.runner.TextTestResult run=452 errors=0 failures=1> error: Test failed: <unittest.runner.TextTestResult run=452 errors=0 failures=1> The command "coverage run setup.py test" exited with 1.Not sure what unittest.runner.TextTestResult is.
It's a generic class from the unittest module
https://docs.python.org/3/library/unittest.html#unittest.TextTestResultDo you have the full output of the test command, or a traceback?
The functionality that you outlined above sounds good to me.
In terms of the figure title: my recollection is that this is updated to show the current gains => we probably don't want to get rid of that functionality completely. We also need to check to make sure that the new functionality works within
sisotool, sinceroot_locusshows up there as well.Finally, note that there are some changes to
root_locus_plotin PR #327 in the way that it handles arguments. I am letting that PR sit for a bit, but will probably merge in a week. If that PR gets merged before yours, you may have to update some of the code for handling function arguments (it shouldn't conflict in any complicated way, but will probably be something more complicated thangitcan handle on its own).Okay thanks for the feedback.
- I will look into that title updating to see if that is a problem. But ideally I think updated information should be inside the plot not in the window name.
- I think I got
sisotoolworking (it passed the test anyway). - I made some updates to
root_locusso I will look at this pull request - thanks. - The bigger problem is
sgrid. At the moment it needs to create the axis so passing the axis may be a problem. I'm looking for some help from the kind people on stackoverflow on that.
@bnavigator thanks, I'm struggling to figure out how to share the Travis CI build log - if that's what you're looking for. Doesn't seem to be an obvious copy or share button. Can you see it at this link?
I am not sure if I selected the correct branch and CI job, but I think this link shows the actual problem:
https://travis-ci.com/billtubbs/python-control/jobs/215952875#L2763====================================================================== FAIL: test_root_locus_zoom (control.tests.rlocus_test.TestRootLocus) Check the zooming functionality of the Root locus plot ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/travis/build/billtubbs/python-control/control/tests/rlocus_test.py", line 68, in test_root_locus_zoom assert_array_almost_equal(zoom_x,zoom_x_valid) File "/home/travis/build/billtubbs/python-control/control/tests/margin_test.py", line 23, in assert_array_almost_equal x[np.isfinite(x)], y[np.isfinite(y)], ndigit) File "/home/travis/miniconda/envs/test-environment/lib/python3.7/site-packages/numpy/testing/_private/utils.py", line 1015, in assert_array_almost_equal precision=decimal) File "/home/travis/miniconda/envs/test-environment/lib/python3.7/site-packages/numpy/testing/_private/utils.py", line 827, in assert_array_compare raise AssertionError(msg) AssertionError: Arrays are not almost equal to 4 decimals Mismatch: 100% Max absolute difference: 5.1 Max relative difference: 1.03727188 x: array([0.1 , 0.1099, 0.1207, 0.1326, 0.1456]) y: array([-5. , -4.6128, -4.1669, -4.0412, -3.9074])... which then fails with the mentioned
TextTestResult. So it is just the test description not set up properly.
https://travis-ci.com/billtubbs/python-control/jobs/215952875#L4138FAILED (failures=1, errors=1, skipped=3) Test failed: <unittest.runner.TextTestResult run=452 errors=1 failures=1> error: Test failed: <unittest.runner.TextTestResult run=452 errors=1 failures=1> The command "coverage run setup.py test" exited with 1.I guess your edits produce unexpected behavior with the rlocus plot zoom test. Given you actually change the behaviour, this means the unittest also needs to be adjusted.
That's it! How on earth do you find that in 3000 lines of log output?
Actually that's not it. It's build 11.4 that I was looking at.
https://travis-ci.com/billtubbs/python-control/jobs/215976746Same failure:
https://travis-ci.com/billtubbs/python-control/jobs/215976746#L1447Just search for FAIL, that string only occurs 11 times.
My browser won't show me the search results.... But I just discovered the 'show raw log as text file' button. Thanks! (First time I've used Travis).
Okay, sorry for the delay @murrayrm I got the
sgrid()method working.Background: The problem with sgrid is that the curvilinear axis can't be super-imposed on an existing axis. You have to delete any existing axis and re-create a new one using the special
mpl_toolkits.axisartist.axislines.Axesclass which is a substitute for the standardAxes. Thanks to a helpful stackoverflow suggestion on how to do this I now have the following behaviour working in my branch:>>> from control import rlocus, tf >>> from control.grid import sgrid, zgrid >>> import matplotlib.pyplot as plt >>> H = tf([2, 5, 1], [1, 2, 3]) >>> fig, axes = plt.subplots(1, 2, figsize=(8,4)) >>> axes[1] = sgrid(ax=axes[1]) >>> r, k = rlocus(H, ax=axes[0]) >>> r, k = rlocus(H, ax=axes[1]) >>> plt.show()The example's is a bit complicated but it produces two root-locus plots side-by-side, one with the standard grid and the second with an sgrid. See image 1 here.
The Matlab way to make an sgrid is as follows:
>> H = tf([2 5 1], [1 2 3]) >> rlocus(H) >> sgrid()This produces one plot like image 2 here.
The main differences are:
- In Matlab, the plot appears immediately after the
rlocuscommand is executed. The sgrid appears on the plot after that. - In python, nothing appears until you execute
plt.show. - In python, if you execute the
sgrid()command after putting a plot on the axis, it gets deleted because it creates a new (empty) axis (which is returned by the method).
I should add. You can still do a simple one-axis plot in python like this:
>>> H = tf([2, 5, 1], [1, 2, 3]) >>> sgrid() # Must be before any plotting starts >>> r, k = rlocus(H) >>> plt.show()If this all looks okay, I'll move on to checking sisotool works with this (should be easier as sisotool will now have more control over the root locus plot).
- In Matlab, the plot appears immediately after the
I only noticed now that Matlab has separate functions for handling plot manipulation - very similar to what we are trying to do here with
zgridandsgrid.Example:
rlocusplot- Plot root locus and return plot handleh = rlocusplot(sys) rlocusplot(sys,k) rlocusplot(sys1,sys2,...) rlocusplot(AX,...) rlocusplot(..., plotoptions)For
pzmapthere is pzplot:h = pzplot(sys) pzplot(sys) pzplot(sys1,sys2,...,sysN) pzplot(ax,...) pzplot(...,plotoptions)I wasn't aware of these at the start so went down the route of allowing passing of an existing axis to
rlocusandpzmapto allow subsequent customization, saving of the plot etc.Does this change that decision? I'll have a think about this but maybe I should revert to having
rlocusandpzmapjust show a stand-alone plot and then implementrlocusplotandpzplotfor controlled figure / axis manipulation.rlocusplotandpzplothaven't been implemented yet in this project as far as I can tell.Will respond in more detail in a bit, but as a general principle we want to by "pythonic" versus MATLAB centric. We can use the
control.matlabsubpackage to provide a more MATLAB-like interface if desired, but our target should be Python users who want to do control design...Perhaps worth looking at how other packages that build on
matplotlibdo things, includingscipy.signal(assuming it has functions that generate plots).Thats good direction to give, thanks. That means we can think out of the box a bit more in terms of a holistic solution to all these methods working effectively:
zgridsgridpzmap/pzplotrlocus/rlocusplotsisotool(usesrlocus)bode/bode_plotnyquist_plotgangof4_plotnichols_plotnichols_gridphase_plot
Are there other methods that make plots that I need to add to this list?
I can't find any plotting functions in
scipy.signal. But they do have this idiom which is interesting:freqz(b, a=1, worN=512, whole=False, plot=None, fs=6.283185307179586) Compute the frequency response of a digital filter. ... Parameters ---------- ... plot : callable A callable that takes two arguments. If given, the return parameters `w` and `h` are passed to plot. Useful for plotting the frequency response inside `freqz`.I'm looking for an example that illustrates the use of this
plotargument.Oh, I see we have already implemented
freqzwith this same argument.You already mentioned the Pandas way of providing
.plot()methods to the data objects. I would consider this together with kwargs such asax=andgrid='s'to be the most pythonic.If you want to retain the MATLAB way of calling sgrid() after creating the plot, I have added a comment at StackOverflow with another idea to add the grid axis.
Using the pylab API in matplotlib has been deprecated for a reason, though. Likewise python-control should not try to stick to that coding paradigm either.
Reacted by Bill TubbsB.t.w. I haven't forgotten about this. Had to focus on another project. Hopefully the delay is not impacting the project.
While debugging some code I noticed that
examples/pvtol-nested.pywas producing a blank figure instead of a pole-zero map that was expected. I chased this down tto the fact that thesgrid()function callsmatplotlib.pyplot.figure(). This is in contrast to all other plotting routines and the consequence is that you get a new set of axes. This causes the following code (fromexamples/pvtol-nested.py) not to work as expected:Figure 10, which should have the output of
pzmapwill be blank sincepzmapcallsfigure()(throughsgrid) and creates a new set of axis (figure #11). Figure #11 will then get erased (away goes the pole/zero map) and be replace by the Gang of 4.