coverage: 94.744% (-0.008%) from 94.752%
when pullingfc1854e on lkies:streamplot
intof73e893 on python-control:main.

This sounds like a great addition. Can you post a few screenshots that show what the new plots look like (since they will be a bit different than thematplotlib examples?

A few thoughts:

• Presumably we don't want to turn on streamlines and streamplots at the same time. Perhaps set things up so that ifplot_streamlines is not True, then settingplot_streamplots will turnplot_streamlines to False?
• I don't think it will break cost to havecplt.lines be an array with an extra element. But you could also just put the data into whatever element of the arrayplot_streamlines uses, since they are sort of the same type of data?
• There are some new documentation guidelines coming in PRUpdated documentation #1094, and it would be good to adhere to those, so that we stay consistent. The main one that I noticed is that you should put objects with documentation (likeNonlinearIOSystem) in single backticks, so that they generate a link. You can find the upcoming developer noteshere.

Regarding the documentation, just to be clear because it will look inconsistent for now: I only change the docs for what i added so it merges properly with the other changes in#1094 ?

Another thing I was not sure about is putting the new parameter to the end ofphase_plane_plot because that could potentially be breaking for cases where the arguments are passed as positional arguments instead of as keywords (which is probably pretty unlikely in this case). Maybe it would be good to make more use of* to make a lot of things like these keyword only?

I am not quite sure I understand you correctly but the way you described the exclusion with streamlines does not seem possible sinceplot_streamlines is true by default. But I also contemplated something similar since it is a bit akward to use this way.
My suggestion would be to defaultplot_streamlines, plotplot_vectorfiled andplot_streamlines to None.
If all Nones are received the function defaults toplot_streamlines=True in any other case the parameters are evaluated as is. With this, if you passplot_streamlines=True,plot_streamlines is automatically disabled but it is still possible to plot them both at the same time in case you would want that (maybe to hightligh specific streamlines or something?).

I would also take the opportunity to increase the grid resolution ifplot_streamlines andplot_vectorfield are disabled, whilestreamplot turns out surprisingly good even with a low resolution it still looks better and is computationally acceptable IMO.

Another problem I realized is that the norm kind of assumes that the plot will use an equal aspect ratio. So if the derivatives are differently scaled (and the plot has a strongly "non equal" aspect ratio) the larger state will dominate, although handling this might be too much for a convenience function like this.

Here are two examples:

importcontrolasctimportnumpyasnpimportmatplotlib.pyplotaspltplt.rcParams['figure.figsize']= [6,6]defdydt(t,y):returnnp.array([y[1],-np.sin(y[0])])ct.phase_plane_plot(dydt,plot_streamlines=False,plot_streamplot=dict(vary_linewidth=True),pointdata=[-4,4,-4,4],gridspec=(20,20))plt.gca().set_aspect('equal')

The linewidths vary from 0.25 to 2 times the current default linewidth, arguably a matter of opinion but I don't think this should be exposed as arguments.

ct.phase_plane_plot(dydt,plot_streamlines=False,plot_streamplot=dict(vary_color=True),pointdata=[-4,4,-4,4])plt.gca().set_aspect('equal')

Here you can see the unfortunate z-ordering with the seperatrices that I talked about. While this looks surprisingly good for only 9 by 6 points, I still think the default resolution should be higher.

But as you can see, it looks pretty good without having to think much about how to distribute the initial conditions.

Regarding the documentation, just to be clear because it will look inconsistent for now: I only change the docs for what i added so it merges properly with the other changes in#1094 ?

Sounds good.

Another thing I was not sure about is putting the new parameter to the end ofphase_plane_plot because that could potentially be breaking for cases where the arguments are passed as positional arguments instead of as keywords (which is probably pretty unlikely in this case). Maybe it would be good to make more use of* to make a lot of things like these keyword only?

We'll probably wait until v0.11 to starting using* to enforce keyword only arguments, since this could break lots of code. For now, OK to assume thatplot_streamlines andplot_streamplot will only be used as keyword arguments.

I am not quite sure I understand you correctly but the way you described the exclusion with streamlines does not seem possible sinceplot_streamlines is true by default. But I also contemplated something similar since it is a bit akward to use this way. My suggestion would be to defaultplot_streamlines, plotplot_vectorfiled andplot_streamlines to None. If all Nones are received the function defaults toplot_streamlines=True in any other case the parameters are evaluated as is. With this, if you passplot_streamlines=True,plot_streamlines is automatically disabled but it is still possible to plot them both at the same time in case you would want that (maybe to hightligh specific streamlines or something?).

This lines up with what I was thinking, though it might also make sense to havestreamplot be the default (I think it looks quite good!).

I would also take the opportunity to increase the grid resolution ifplot_streamlines andplot_vectorfield are disabled, whilestreamplot turns out surprisingly good even with a low resolution it still looks better and is computationally acceptable IMO.

Seems fine. Computation is not a major issue here (and if someone has some complicated vector field that takes a long time to compute, they can always change the grid resolution.

Another problem I realized is that the norm kind of assumes that the plot will use an equal aspect ratio. So if the derivatives are differently scaled (and the plot has a strongly "non equal" aspect ratio) the larger state will dominate, although handling this might be too much for a convenience function like this.

Probably OK for now. If needed, we can add some functionality to change the aspect ratio inphase_plane_plot.

@lkies The big documentation PR has been merged, so you'll need to rebase this on top of the latest main.

I implemented the thing with theNones but I did not setplot_streamplot as the default yet, even though I also (unsurprisingly) like it better. I didn't do this for two reasons:

• I didn't want to update all of the examples (I might look into that later this week)
• It will makecontrol/tests/ctrlplot_test.py::test_plot_linestyle_processing[None-phase_plane_plot] fail for (in my opinion) pretty stupid reasons: The test checks the color of the first line in the flattenedControlPlot structure, but the first one it fines is a line fromplot_separatrices which does not respect the default color so it fails. The reasons it did not fail before, was because the streamlines appear earlier in the structure. Even if I could work around this order issue,streamplot returns aStreamplot object and not lines. Except for putting in a dummy line or disabling the test, I do not really see a way to get the test to pass.

I also get 6 other failed tests, but they also fail on main for me so it should be unrelated.

FAILED doc/test_sphinxdocs.py::test_sphinx_functions[control] - Failed: control.ControlPlot not referenced in sphinx docsFAILED doc/test_sphinxdocs.py::test_sphinx_functions[control.flatsys] - Failed: control.flatsys.BSplineFamily not referenced in sphinx docsFAILED doc/test_sphinxdocs.py::test_sphinx_functions[control.optimal] - Failed: control.optimal.OptimalControlProblem not referenced in sphinx docsFAILED doc/test_sphinxdocs.py::test_sphinx_functions[control.phaseplot] - Failed: control.phaseplot.NonlinearIOSystem not referenced in sphinx docsFAILED doc/test_sphinxdocs.py::test_config_defaults - FileNotFoundError: [Errno 2] No such file or directory: 'config.rst'FAILED doc/test_sphinxdocs.py::test_sphinx_matlab - Failed: control.matlab.InputOutputSystem not referenced in sphinx docs

So depending on if you actually want to have the default changed, which is technically a breaking change, I would consider this done, so I unmarked it as draft.

In terms of updating the documentation: no matter what we need to update the user guide to saysomething aboutstreamplot, otherwise the only people who will know it exists are people who read the docstrings. So a paragraph needs to be added todoc/phaseplot.rst no matter what. The plots in that section are generated by the code in that section, so "updating" the plots is just a matter of putting in the righttestcode (see other examples in that file).

If you want to leave the default asstreamlines, then just a simple paragraph with a simple example (along the lines of the ones above) is all that is needed.

In terms of the failing tests: what you are showing above is different that what the CI tests are showing. They show the only problem is that you haven't documentedstreamplot in the Reference Manual. That is easy to fix: just go to thephaseplot section offunctions.rst and addphaseplot.streamplot.

I'll do a more careful review of the code in the coming days.

Ok, I will do that. And thank you for the tip regarding the failing CI test, at first glance I was unsure, whether that file was autogenerated by some process.

Do you have a suggestion regarding the test that will fail when changing the default?

For the unit tests: all we really need is something that executes all of the lines of your code (for coverage) and that will generate an error if the lines don't plot. So perhaps choose some simple set of settings that are easy to check? (And of course it should not work if somehowplot_streamlines were used instead ofplot_streamplot.)

One more thought: if we makestreamplot the default, how hard would it be to add the ability to set the depth of other objects (eg, separatrices) so that they appear on top of the stream plot? We could make that a common keyword argument for the other plotting functions inphaseplot and then set the defaults so that the plots look right.

Ensuring that the separatrices and stationary points are always on top is not too bad, I just tried it and will push it later or tomorrow. I just get the maximum zorder of streamlines, vectorfield or streamplot and then add something to it for seperatrices and equilpoints unless their zorder is already specified. And then the same for equilpoins over separatrices. Although it only requires exposing the zorder for separatrices and equilpoints I would expose it for all 5 functions for consistency.

I am not sure if you just wanted to remind me to write tests, but the thing I said about the tests, was that if I set streamplot as the default it would fail the mentioned test (control/tests/ctrlplot_test.py::test_plot_linestyle_processing[None-phase_plane_plot])
So I will need to exclude streamplot from one of the parametrized tests and write a specialized one that checks for the correct color in the right place. Or do you have a better idea?

For unit tests, I would keep all current tests, just restructure so that they test with the relevant options. Then add a new test forstreamplot and use that test for the default test. I'm not sure how much work that is. The main thing is that we need to make sure that all lines of code are covered, since otherwise if future changes break the plots, we don't find out until someone reports it.

I changed the default plot type to streamplot, implemented the automatic zordering and wrote some tests.
As you can see, the examples currently fail because I have not yet updated them, so they still assume thatplot_streamlines is the default.
I will work on that sometime in the next few days.

I also noticed thatseparatrices seems to also plot the equilibrium points, which I found a bit odd since that should be the job ofequilpoints.

I agree thatphaseplot.separatrices should not be plotting equilibrium points. Feel free to fix that~

As a fix for the examples, I suggest putting the following code at the top ofphase_plane_plots:

    # Check for legacy usage of plot_streamlines    streamline_keywords = [        'arrows', 'arrow_size', 'arrow_style', 'color', 'dir', 'params']        if plot_streamlines is None:        if any([kw in kwargs for kw in streamline_keywords]):            warnings.warn(                "detected streamline keywords; use plot_streamlines to set",                FutureWarning)            plot_streamlines = True        if gridtype not in [None, 'meshgrid']:            warnings.warn(                "streamplots only support gridtype='meshgrid'; "                "falling back to streamlines")            plot_streamlines = True

Also, something goes wrong in this example that I don't yet understand:

def invpend_update(t, x, u, params):    m, l, b, g = params['m'], params['l'], params['b'], params['g']    return [x[1], -b/m * x[1] + (g * l / m) * np.sin(x[0])]invpend = ct.nlsys(    invpend_update, states=2, inputs=0,    params={'m': 1, 'l': 1, 'b': 0.2, 'g': 1})ct.phase_plane_plot(    invpend, [-2*np.pi, 2*np.pi, -2, 2], 4, gridspec=[6, 6],    plot_separatrices={'timedata': 20, 'arrows': 4})

If you look at the plot that is generated, the streamlines cross the separatrices (?):

I am pretty sure the weird behavior is just a resolution problem, it becomes more obvious when plotting the vectorfield:

definvpend_update(t,x,u,params):m,l,b,g=params['m'],params['l'],params['b'],params['g']return [x[1],-b/m*x[1]+ (g*l/m)*np.sin(x[0])]invpend=ct.nlsys(invpend_update,states=2,inputs=0,params={'m':1,'l':1,'b':0.2,'g':1})ct.phase_plane_plot(invpend, [-2*np.pi,2*np.pi,-2,2],4,gridspec=[6,6],plot_separatrices={'timedata':20,'arrows':4},plot_vectorfield=True)

The vectorfield that you see here is basically the only information thatplt.streamplot gets and needs to construct the streamlines from. And as you can see, it really is not a lot of data, if you increase the resolution the lines will match up again:

ct.phase_plane_plot(invpend, [-2*np.pi,2*np.pi,-2,2],4,gridspec=[30,20],plot_separatrices={'timedata':20,'arrows':4})

I guess this might be somewhat of a "sharp" edge, that even with very low resolution dataplt.streamplot will always produce "good" looking plots but due to the low resolution they might be inaccuare.

The somewhat jagged seperatrices are also a resolution problem, 20 units is quite a long time to spread the 50 points for which it will calculate the streamline for.

I added the legacy warning / fallback and updated the guide withplot_streamplot as the new default andplot_streamlines for advanced usage. I did not includecolor andparams in the list of keyword arguments as they are also used and supported by the other functions. I also explicitly addedplot_streamlines=True for all calls inexamples/phase_plane_plots.py.

Is there anything still missing?

@lkies I created a PR into your repository with a few modifications so that the code and documentation style is consistent with usage throughout the rest of the package. I also updated some of the example scripts to use the new functionality in the appropriate places.

If the PR in your repository looks OK, go head and merge it. Once it shows up here, I'll merge this PR into the main branch.

Great work overall, and I'm looking forward to having this be the default functionality!

@murrayrm No problem, happy to have contributed!
Thank you for your fixes, there was one slight error in the documentation, though that seems to have been my fault originally.