-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Make Spines accessable by the attributes. #17099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I‘m +0.5 on this. Slightly concerned about the redundancy (there should only be one way...), but I think attribute access is easier and more natural than item access.
You should override __dir__
as well to include the names.
I'm only -0.5 on this, but if we do take this we should make sure that we populate On the pro-side I see the argument for the slightly terser access (4 less chars on 3-5 char names) and it does make the access match the rcparams. On the con-side, in my day-job I have ended up with dictionary sub-classes, in part to support dot-access like this, and ended up regretting it. We ran into a bunch edge cases where the we subtly changed the behavior of the base-class and our sub-class (which was doing more than adding dot-access). There is also the slightly increased complexity of explaining "this is mostly an A bigger concern is that we have documented over-riding
|
- change `__dir__` of OrderedDict
|
I want to mention that the fact the
Not having to remember the name of each spine is a great advantage. (Similar for a loop, of course!) That being said, I think the main problem is that the |
If my understanding is correct, the functional programming scheme for plotting is a 'old' way, but now it is more inclined to adopt an object-oriented approach. Like other of the plt.xxx function in the package, It's not difficult to imagine that scheme like ax.spines.set_visible(False) or ax.spines.visible = False would be easier to be accept by new users, than plt.setp(ax.spines.values(), visible=False) Also, a plt.setp(ax.spines, visible=False) BTW, Controlling the spines is one of the few features that matplotlib surpasses ggplot, while it is not well documented...😂 |
Recommending to override a private method seems to a good choice, but could gracefully be handled with:
I think that's ok as long as that handles all reasonable input well.
It should be viable to make
where IMHO that would be a better broadcasting API than
|
Are we overcomplicating things? (Given every 2h-python-crash course teaches you how to use a dictionary) |
IMHO not. The dictionary API is a somewhat unusual way of accessing a rather fixed set of subcomponents. And the method for the very common case of modifying all spines is not quite obvious. |
The thing we really want to move away from is the implicit global state of pyplot (
The names are customized by the individual axes so while we ship a relatively constrained set, I don't think we should put a technical limitation on what names can come in. This code also likely dates back to the early days of the library when we were worrying about things like py24 compatibility. We definitely should not break the API of 'spines as a dict' (even if we stop using it internally and un-document it as much as possible) so we don't break user code. While we do change the API, we try to only do that in cases where the existing behavior was problematic.
This an important point. At my day-job we did the attribute access in way that also made key access fallback to attribute access so @yech1990 Thank you for working on this, that you have gotten this much of a response (even if it leans negative) is a sign you are pushing on an interesting and useful feature ! Please don't be discouraged. |
@timhoffm @ImportanceOfBeingErnest Can one of you please volunteer to champion this PR through the process (either getting it merged or deciding this is not something we want to do)? |
I'm sorry, I don't see championing this one occur anywhere towards the top of my priority list. |
Thank you @tacaswell, @ImportanceOfBeingErnest and @timhoffm .I learned a lot from your comments. This PR is just a dirty demo, #17107 provide a much a brilliant design. The reason that I ask for a I used to have such questions. I don't think I have the ability to provide useful help, but I look forward to using these functions in matplotlib more conveniently in a future version. |
Thank you @yech1990 Hopefully we will hear from you again! |
PR Summary
Make Spines accessable by the attributes.
PR Checklist