Skip to content

Make Artist.set() apply properties in the order in which they are given. #16328

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 6, 2020
Merged

Make Artist.set() apply properties in the order in which they are given. #16328

merged 1 commit into from
May 6, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 25, 2020

... with a deprecation period (see changelog).

  • This is consistent with Artist.update, and we don't reorder separate
    calls to set_color and set_edgecolor either (we can't really to that,
    indeed).
  • The _prop_order mechanism was a slightly complex machinery for a
    single use case (applying "color" first).
  • Writing p.set(edgecolor="r", color="g") seems a bit twisted anyways.

Also rewrote (and simplified) normalize kwargs to make it also
maintain kwarg order, as that's necessary to make the warning emitted in
the correct cases.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added this to the v3.3.0 milestone Jan 25, 2020
@anntzer anntzer mentioned this pull request Feb 12, 2020
Closed
@ImportanceOfBeingErnest
Copy link
Member

I would think that doing the inverse, namely to ignore the order and instead always evaluate the parameters in the same hierarchical order would be much cleaner. Meaning, I don't think the following two


set(edgecolor="g", color="b")
set(color="b", edgecolor="g")

should ever result in a different output.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 12, 2020

FWIW in #5599 (comment) it was explicitly chosen that Artist.update(props) (which is the same as set(), except that the dict is not unpacked) should not reorder properties (which back then mostly mattered if props was an OrderedDict) -- e.g. update(OrderedDict([("color", "k"), ("edgecolor", "g")])) would behave differently from update(OrderedDict([("edgecolor", "g"), ("color", "g")])); I think it's quite clear that the only reason the same approach was not used for set() back then is that it's only since py36 that we even have info about the order of kwargs.

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Feb 12, 2020 •
edited
Loading

My point is that for a user-facing API, it's quite unusual to rely on ordering. And if you create your parameters dynamically, you might not even know their order (e.g. a downstream library assembling properties on the fly, possibly allowing users to pass in further kwargs).
Instead what the user needs is a clear set of rules for conflicting cases, e.g. edgecolor beats color, original beats alias (or vice versa), that are applied consistingly throughout the library.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 12, 2020

original doesn't beat alias -- we error out if both are passed.
Would you argue that the behavior of update() needs to be changed, in that case? Where do you propose to document the priority order between properties?

@ImportanceOfBeingErnest
Copy link
Member

original doesn't beat alias -- we error out if both are passed.

That's fine. Although a clear rule would make people's lifes easier, if e.g. they could just kwargs.update(horizontalalignment="left") and be sure that the result is left aligned without checking the content of kwargs first.

Would you argue that the behavior of update() needs to be changed, in that case?

I guess that would be the consequence, yes. If some API is going to break for 4.0 anyways, this might be a good point to do it.

Where do you propose to document the priority order between properties?

Many docstrings are interpolated anyways, so that should be the least of a problem.

In general, I just think this warrants a little more discussion - possibly there are good arguments against that proposal, which I don't see at the moment.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 12, 2020

For me, what really matters is consistency and clarity.
wrt consistency: I think it's better to align set() with update(), but if you want to instead make update() aligned with set(), go for it :)
wrt clarity: I think "properties are applied in the order in which they are given" is clearer and simpler to explain than "properties are applied according to the priority order which is listed in ...", but again, I'm not religious about that.

@anntzer anntzer added the status: needs comment/discussion needs consensus on next step label Feb 14, 2020
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 1, 2020
@anntzer anntzer mentioned this pull request May 6, 2020
Merged
6 tasks
efiring
efiring approved these changes May 6, 2020
View reviewed changes
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I can understand the argument for establishing some fixed ordering, but "put them in the order you want them" is so much simpler, and fits the idea of set() as a shorthand for listing a sequence of clunky set_this function calls.

@efiring efiring modified the milestones: v3.4.0, v3.3.0 May 6, 2020
@efiring
Copy link
Member

efiring commented May 6, 2020

Needs rebase, but I don't see any reason not to put this into v3.3, so I moved up the milestone.

@anntzer
Make Artist.set() apply properties in the order in which they are given.
71d725f 
... with a deprecation period (see changelog).

- This is consistent with Artist.update, and we don't reorder separate
  calls to set_color and set_edgecolor either (we can't really to that,
  indeed).
- The _prop_order mechanism was a slightly complex machinery for a
  single use case (applying "color" first).
- Writing `p.set(edgecolor="r", color="g")` seems a bit twisted anyways.

Also rewrote (and simplified) normalize kwargs to make it *also*
maintain kwarg order, as that's necessary to make the warning emitted in
the correct cases.
@anntzer
Copy link
Contributor Author

anntzer commented May 6, 2020

rebased

@efiring efiring merged commit a57ea3d into matplotlib:master May 6, 2020
@anntzer anntzer deleted the setkworder branch May 6, 2020 07:37
@QuLogic QuLogic removed the status: needs comment/discussion needs consensus on next step label Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efiring efiring efiring approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@anntzer @ImportanceOfBeingErnest @efiring @timhoffm @QuLogic