Two 'sgrid' functions when we only need one? #634
Description
Activity
I'm planning to look at this tomorrow. Does anyone know why
rlocus.pycalls its own local_sgrid_func(fig=None, zeta=None, wn=None)instead ofgrid.sgrid()(which is already imported into the module but is not used). There might be a reason why someone did this...See these lines:
python-control/control/rlocus.py
Lines 234 to 239 in 6b7a9c7
I'm planning to look at this tomorrow. Does anyone know why
rlocus.pycalls its own local_sgrid_func(fig=None, zeta=None, wn=None)instead ofgrid.sgrid()It is basically a result of two PRs which were started indepently and merged after some time: #617 (comment)
Okay, thanks. So more of an accident than an intention.
Okay, I've run into the same problem from last time I looked at this. The
sgrid()function is not compatible with the plotting paradigms we wanted to employ for figures with multiple subplots:plt.subplot(2, 1, 1) zgrid() # this works plt.subplot(2, 1, 2) sgrid() # this won't plt.show()
and
fig, axes = plt.subplots(2, 1) zgrid(ax=axes[0, 0]) # this works sgrid(ax=axes[1, 0]) # this won't plt.show()
This won't work because
sgriduses aPolarAxes.PolarTransformto create the axis, which you have to do at the point of creating the axis, not after one already exists:python-control/control/grid.py
Lines 88 to 93 in 6b7a9c7
Also, I don't think there's a way to remove an existing axis and then replace it with the desired polar axis (could be wrong on this point).
Welcome comments on this but I think there is one solution...
The only solution I can think of right now is to require a slightly different set of arguments when adding an
sgridto an existing figure. Based on this example script from the documentation, I think this might work but I haven't tried implementing it yet:fig = plt.figure() sgrid(fig=fig, position=(2, 1, 1)) # this should be do-able zgrid(fig=fig, position=(2, 1, 2)) # add the same functionality to zgrid plt.show()
From the documentation, it sounds like the subplots method was intended as a convenience method for making plots of the same type only. It does have two keyword arguments for creating different types of axes and grids, subplot_kw and gridspec_kw but I'm not sure you can specify different types for each subplot (I will check this).
Question. I'm in the process of replacing
_sgrid_funcwithsgridand I noticed that_sgrid_funcuses two other functions inrlocus.py:python-control/control/rlocus.py
Lines 712 to 718 in 6b7a9c7
python-control/control/rlocus.py
Lines 750 to 754 in 6b7a9c7
Shall I delete these functions (they were only used by
_sgrid_func) or should these features be carried over?- Hi bill, I’ve never seen them used anywhere else, and as private functions I think you can delete them if they don’t show up in anywhere else in the library.On Sun, Aug 15, 2021 at 2:54 PM Bill Tubbs ***@***.***> wrote: Question. I'm in the process of replacing _sgrid_func with sgrid and I noticed that _sgrid_func uses two other functions in rlocus.py: https://github.com/python-control/python-control/blob/6b7a9c78f16ab04e009be7155308294080d8a5d1/control/rlocus.py#L712-L718 https://github.com/python-control/python-control/blob/6b7a9c78f16ab04e009be7155308294080d8a5d1/control/rlocus.py#L750-L754 Shall I delete these functions (they were only used by _sgrid_func) or should these features be carried over? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#634 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN74SSONX6UL556R6PH4OGDT5AZPNANCNFSM46YOSZAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .-- Sawyer Fuller Assistant Professor of Mechanical Engineering Adjunct, Paul G Allen School of Computer Science University of Washington http://faculty.washington.edu/~minster/ (Typed with my thumbs)Reacted by Bill Tubbs
fig, axes = plt.subplots(2, 1) zgrid(ax=axes[0, 0]) # this works sgrid(ax=axes[1, 0]) # this won't plt.show()
fig = plt.figure() sgrid(fig=fig, position=(2, 1, 1)) # this should be do-able zgrid(fig=fig, position=(2, 1, 2)) # add the same functionality to zgrid plt.show()
You can always get the parent figure of a given axis by
ax.get_figure()and the position byax.get_subplotspec(). So whatever you can achieve by the second form should also be possible with the first form.This won't work because
sgriduses aPolarAxes.PolarTransformto create the axis, which you have to do at the point of creating the axis, not after one already exists:python-control/control/grid.py
Lines 88 to 93 in 6b7a9c7
Also, I don't think there's a way to remove an existing axis and then replace it with the desired polar axis (could be wrong on this point).
The MPL Curvilinear grid demo where this was taken from has changed a little bit since then. Maybe this needs an update too.
You can always get the parent figure of a given axis by ax.get_figure() and the position by ax.get_subplotspec(). So whatever you can achieve by the second form should also be possible with the first form.
The problem is not getting access to the figure, it is that the axes has to be created in a certain way by
sgrid.sgridcannot work with an axis that is already created so it has to be a blank figure (before the axis was created). Unless someone knows a way to remove an axis already created so it can be replaced.I am not sure about this. Maybe you can shuffle around existing axes objects together with a newly created parasite axis using the right combination of
delaxes(ax),fig.add_subplot(ax), etc.Reacted by Bill TubbsThanks,
delaxes(ax)looks promising. I will check out later this week.
As pointed out in PR #617, there are now two
sgridfunctions in the package:grid.py:sgridandrlocus.py:_sgrid_func. It looks like_sgrid_funcwas removed in PR #193 (discrete omega-damping plot and tweaks by Sup3rGeo), but was re-introduced in PR #209 (sisotool and dynamic root locus zoom by icam0).