Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

efiring
Copy link
Member

@efiring efiring commented Jun 3, 2023

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

@efiring efiring marked this pull request as draft June 4, 2023 00:22
@efiring efiring mentioned this pull request Jun 4, 2023
7 tasks
@efiring efiring marked this pull request as ready for review June 4, 2023 07:31
Copy link
Member

@story645 story645 left a 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?

@timhoffm
Copy link
Member

timhoffm commented Jun 4, 2023

I'm -0.8 on this. There is a difference between the user API (set_facecolor()) and the internal data represenation. While a user can use set_facecolor('red'), we convert it into a (1, 4) array. The plural makes it clear that it is an iterable over the colors (which, yes, may contain only one element). Also internal expressions like len(self._facecolors) read better than len(self._facecolor).

@efiring efiring mentioned this pull request Jun 5, 2023
1 task
@story645
Copy link
Member

story645 commented Jun 5, 2023

There is a difference between the user API (set_facecolor()) and the internal data represenation

But get_facecolor is gonna return __facecolors so if someone needed the private variable for whatever reason, that's what they'd have to pull. I think 'especially since we have the color/colors mean different things API, it's a lot clearer when reading the code if the internal matches the public - otherwise I'd be double checking wait do we have another color parameter in the signature?

@timhoffm
Copy link
Member

timhoffm commented Jun 5, 2023

I cannot follow your argument. facecolors is an alias to facecolor. These are equivalent for the public API. There'll be always one of the two that won't match the internal naming. In fact, Collection.__init__ even uses the plural versions.

self._shading = shading
self._bbox = transforms.Bbox.unit()
self._bbox.update_from_data_xy(self._coordinates.reshape(-1, 2))
kwargs["antialiased"] = antialiased
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
kwargs["antialiased"] = antialiased

I think you can remove this, and remove antialiased from the signature above and then it is in kwargs already.

@story645
Copy link
Member

story645 commented Jun 5, 2023

@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).

@efiring
Copy link
Member Author

efiring commented Jun 5, 2023

@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.

@efiring
Copy link
Member Author

efiring commented Jun 5, 2023

The plural can be pretty ugly, too: "antialiaseds"? Really? And in general that can be plural for collections, but it is singular in QuadMesh.

@timhoffm
Copy link
Member

timhoffm commented Jun 5, 2023

now I'm annoyed at the public API and feel like the singular should get discouraged

OTOH the singular versions are inherited from Artist.

On the public API we're torn between

  • the "singular" inheritance from Artist
  • the somewhat justifiyable plural semantics
  • the constructor arguments
  • the aliasing

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 CircleCollection(..., facecolor="red") and CircleCollection(..., facecolor=["red", "green", "blue"]) work.

Internal naming would depend on normalization: If we carry the element around as is, it would be the public name (self._facecolor="red" / self._facecolor=["red", "green", "blue"]). If we normalize the parameter to an iterable I'd go with the plural self._facecolors.

@efiring
Copy link
Member Author

efiring commented Jun 7, 2023

In another PR the public API could be nudged toward the singular by changing examples, docstrings, kwarg defaults and the alias table.

@tacaswell tacaswell added this to the v3.8.0 milestone Jun 8, 2023
@ksunden ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023
@efiring efiring force-pushed the collection_private_singular_1 branch from 5616554 to ce900fd Compare October 13, 2023 01:53
Copy link
Member

@dstansby dstansby left a 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.

@efiring
Copy link
Member Author

efiring commented Dec 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants