-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Make private collection attributes singular #26062
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
base: main
Are you sure you want to change the base?
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.
Since this is private, does this not need an API note?
I'm -0.8 on this. There is a difference between the user API ( |
But get_facecolor is gonna return |
I cannot follow your argument. |
lib/matplotlib/collections.py
Outdated
self._shading = shading | ||
self._bbox = transforms.Bbox.unit() | ||
self._bbox.update_from_data_xy(self._coordinates.reshape(-1, 2)) | ||
kwargs["antialiased"] = antialiased |
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.
kwargs["antialiased"] = antialiased |
I think you can remove this, and remove antialiased from the signature above and then it is in kwargs
already.
@timhoffm sorry should have double checked and now I'm annoyed at the public API and feel like the singular should get discouraged (the three ways to access the same thing is giving me hives). |
@timhoffm, I think that I might have been the originator of the plurals long ago, so I am sympathetic to the argument that names for plural things should be plural, but over time there have been complaints about having some things plural and other similar thing singular, and the consensus at the time of #18592 was to shift the private variables to singular, and to use aliases so that collection kwargs could be singular or plural. The aliases were implemented, but not the private variable changes. The singular form was used in a change in mplot3d, however. So right now we have a bit of a mess. On balance, I think that moving everything to favor the singular will reduce confusion. The grammatical ugliness is an unfortunate side effect, but is outweighed by the simplification. Also, note that some of these sorts of variables can be singular or plural for the same object; and something originally designed as singular (like alpha) can end up being extended to be optionally plural. |
The plural can be pretty ugly, too: "antialiaseds"? Really? And in general that can be plural for collections, but it is singular in QuadMesh. |
OTOH the singular versions are inherited from On the public API we're torn between
Overall, it's best to leave everything there as is. In the internals, we can do whatever we want. I have no overview here, but if we have a singular-plural mixture we should definitively have a clear policy and unify this. The direction of the change may be open to discussion. On a side-note: If I was to design this from scratch, the public API would be all-singular. You could have single values used as broadcast Internal naming would depend on normalization: If we carry the element around as is, it would be the public name ( |
In another PR the public API could be nudged toward the singular by changing examples, docstrings, kwarg defaults and the alias table. |
5616554
to
ce900fd
Compare
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 👍 to this in principle, but looks like some tests need fixing first.
Thank you, @dstansby. I botched a rebase, so the current state of this PR is broken. Personally, I still think what it is trying to do is a step in the right direction--though a very minor improvement. @timhoffm, your last comment was not entirely clear to me: are you OK with this internal change, or not? I won't try to fix the PR unless it is clear it will go in. |
PR summary
This replaces the part of #18592 that makes collection private attributes like
_linewidths
and_facecolors
singular (_linewidth
and_facecolor
) to match their getters and setters.PR checklist