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 with plt.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, ax say, that is the matplotlib axis that the sub-function should add the plot to? If ax is None then 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 pzmap and also fix examples/pvtol-nested.py.

This is how Pandas plot functions work I think. See ax argument 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() or plt.show() from within the python-control package, with the possible exception of inside the matlab subpackage (to generate behavior that is consistent with how MATLAB operates).

This fix might be a bit more complicated because some functions (like bode_plot and gangof4_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 _subplots helper 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) where rloc_num is 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.TextTestResult

Do 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, since root_locus shows 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 than git can 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 sisotool working (it passed the test anyway).
• I made some updates to root_locus so 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#L4138

FAILED (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/215976746

Same failure:
https://travis-ci.com/billtubbs/python-control/jobs/215976746#L1447

Just 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.Axes class which is a substitute for the standard Axes. 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 rlocus command 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).

I only noticed now that Matlab has separate functions for handling plot manipulation - very similar to what we are trying to do here with zgrid and sgrid.

Example:
rlocusplot - Plot root locus and return plot handle

h = rlocusplot(sys) 
rlocusplot(sys,k)
rlocusplot(sys1,sys2,...)
rlocusplot(AX,...)
rlocusplot(..., plotoptions)

For pzmap there 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 rlocus and pzmap to 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 rlocus and pzmap just show a stand-alone plot and then implement rlocusplot and pzplot for controlled figure / axis manipulation. rlocusplot and pzplot haven'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.matlab subpackage 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 matplotlib do things, including scipy.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:

• zgrid
• sgrid
• pzmap / pzplot
• rlocus / rlocusplot
• sisotool (uses rlocus)
• bode / bode_plot
• nyquist_plot
• gangof4_plot
• nichols_plot
• nichols_grid
• phase_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 plot argument.

Oh, I see we have already implemented freqz with 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 as ax= and grid='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.

B.t.w. I haven't forgotten about this. Had to focus on another project. Hopefully the delay is not impacting the project.