Coverage increased (+0.07%) to 90.091% when pulling 5e9b179 on sawyerbfuller:indent2 into 5155dd5 on python-control:master.

Nice! Will try to look through the code in the coming days and leave any comments.

Further updates and now ready to merge of ok'd:

• rename maximum_magnitude to plot_maximum_magitude. Option to turn off this plot by setting its value to 0.
• docstring description of behavior
• if and only if omega is specified, the old approach to indentation is used, otherwise indentation points are added.

I've been playing around with an example that did not work quite right in the original code because of the way that the points were generated along the frequency axis (basically points weren't dense enough, so you could miss encirclements around unstable poles at non-zero frequencies). It looks like the new version of the code has some similar problems as well as introducing some new issues that I am not sure I understand yet.

Here is an example (derived from something more complicated) that works with the previous version but does not work with this version:

import numpy as np
import matplotlib.pyplot as plt
import control as ct
import control.matlab as matlab

sys = ct.ss(ct.tf(*matlab.zpk2tf(
    [-3e-5 + 3j, -3e-5 - 3j], 
    [0, 1e-4 + 2j, 1e-4 - 2j, -1e-2], 
    1)))
K, S, E = ct.lqr(sys, np.eye(sys.nstates), 1)
L, P, E = ct.lqe(sys.A, sys.B, sys.C, 1, 1)
ctrl = ct.ss(sys.A - L @ sys.C - sys.B @ K, L, K, 0)
ct.nyquist_plot(sys * ctrl)

This code creates a process sys that has a pole at the origin and two unstable poles at 1e-4 +/- 2j and then creates a stabilizing controller using LQR with an estimator. Since the original system is unstable but the closed loop is stable (easy to check), the loop transfer function should have two CCW encirclements of the -1 point, so nyquist_plot() should return -2 (clockwise encirclements).

Running the original code yields the following plot and a return value of -2:

Running the new code yields the following plot and a return value of 0 (which is not correct):

Based on work I did on my original example, I suspect that the problem is that we are not handling addition of points around the poles at 1e-4 +/- 2j correctly. In particular, note that these not that close to the imaginary axis (so np.isclose will not pick them up), but you need to make sure to have a reasonably dense set of sample points near that frequency or you will missing the "inner" loops in the first figure.

In addition to the wrong contour and the wrong count of encirclements, there is something going on with the arrows that needs to be fixed, I think.

I have a more complicated example where you have to be very careful with the indent_radius in order to get the right answer. I want to test that example as well, but we should probably find fix the errors above first. (We should also add some unit tests that capture the incorrect encirclement count bug.)

@murrayrm what you suggest sounds like a reasonable explanation to me.

So this is a second, unrelated reason why it might be nice to have automatic frequency vector generation code that adds more points near poles (the other being that then the shape of the bode plot can be better visualized).

I have no idea what is going on with those arrows but it may be related to having discontinuous points

A few additional comments after playing around with this a bit:

• Is there a way to have multiple overlapping extra contours offset a bit so that you can see that there are multiple encirclements? The "system with poles not at origin" example above is one case showing why this could be useful (in the original diagram you can see two distinct curves, but in the modified version these curves overlap in a way that is non-obvious).

• Rather than setting plot_maximum_magnitude to zero to turn off the functionality, perhaps set it to None?

• I feel like the default value for plot_maximum_magnitude should be None so that you get a "regular" Nyquist plot by default, since otherwise people might be confused by this non-standard representation.

Also, I've put some preliminary code that does a better job at adding points near poles here. I'm thinking of pulling that code into the _determine_omega_vector function in freqplot.py, in which case it would also provide more resolution in Bode plots.

@murrayrm re mulitple contours: makes sense, good idea.

re turning off plot_maximum_magintude plot: I tried using None first but got stuck because I thinkconfig._get_param interprets a None keyword as meaning that the default should be used.

re default value for plot_maximum_magnitude being off: I agree it may be non-standard, but isn't the previous way also? The difference is the new way allows you to set a reasonable radius to show in response to indentations, rather than have it depend on indent size and the magnitude of the system at that indent.

One alternative to consider: maybe we could stick with the old way, but size each indent according to the size of the residue (or maybe there is some better formula), so that encirclements are always roughly the same size. You would probably need to further adjust the indent for each pole to avoid overlap with other indentations.

This is a tricky interface issue.