Skip to content

Deprecate passing drawstyle with linestyle as single string. #12442

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
Oct 26, 2018
Merged

Deprecate passing drawstyle with linestyle as single string. #12442

merged 1 commit into from
Oct 26, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 8, 2018

Instead of plt.plot(..., linestyle="steps--"), use plt.plot(..., linestyle="--", drawstyle="steps"). ds is now an alias for
drawstyle.

Note that plt.plot(..., "steps--") already doesn't work (due to
confusion as to whether "steps" is a color, a marker or a drawstyle).

The end goal is to simplify and rationalize linestyle specification as
rcParams vs. as arguments (#11797).

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 force-pushed the drawstyle-linestyle branch from d8ee5e6 to f49b106 Compare October 8, 2018 12:12
timhoffm
timhoffm approved these changes Oct 8, 2018
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Minor comments, do with them as you like.

lib/matplotlib/lines.py
cbook.warn_deprecated(
"3.1", message="Passing the drawstyle with the linestyle "
"as a single string is deprecated since Matplotlib "
"%(since)s and support will be removed %(removal)s; "
Copy link
Member

Choose a reason for hiding this comment

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

removed in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

%(removal)s includes it.

lib/matplotlib/lines.py Outdated
"3.1", message="Passing the drawstyle with the linestyle "
"as a single string is deprecated since Matplotlib "
"%(since)s and support will be removed %(removal)s; "
"please pass the drawstyle separately ('drawstyle=...' or "
Copy link
Member

Choose a reason for hiding this comment

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

-0.5 on quoting 'drawstyle=...'. Might lead to confusion with strings. Alternative:

please pass the drawstyle separately using the drawstyle (or ds) keyword arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@anntzer
Deprecate passing drawstyle with linestyle as single string.
d35c3a7 
Instead of `plt.plot(..., linestyle="steps--")`, use `plt.plot(...,
linestyle="--", drawstyle="steps")`. `ds` is now an alias for
`drawstyle`.

Note that `plt.plot(..., "steps--")` already doesn't work (due to
confusion as to whether "steps" is a color, a marker or a drawstyle).

The end goal is to simplify and rationalize linestyle specification as
rcParams vs. as arguments.
@anntzer anntzer force-pushed the drawstyle-linestyle branch from f49b106 to d35c3a7 Compare October 8, 2018 16:53
@NelleV
Copy link
Member

NelleV commented Oct 13, 2018

Why are we adding an alias to keywords?
We should have one and only one way to do things, and if possible give explicit names to arguments and variables. I'm not convince it's a good idea to add more aliases than we already have (I think we should be deprecating those in fact).

I'm 👎 on the ds alias and 👍 on the rest.

@timhoffm
Copy link
Member

timhoffm commented Oct 13, 2018 •
edited
Loading

@NelleV I understand your argument against aliases, and have been on the same page. However, when working interactively a lot, I've come to appreciate, being able to write ls instead of linestyle. OTOH, the drawstyle is likely rare enough that an alias is not really neccessary.

So -0.5 on ds, but +1 on keeping existing aliases.

@jklymak
Copy link
Member

jklymak commented Oct 13, 2018

I’m good w the alias. Even if you script, not typing “linestyle” each time would be a nice convenience.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 13, 2018

Whether we should fully get rid of aliases is a good question (I agree with @jklymak that they have their usefulness) but I think it's better to be consistent and provide aliases for everything for now rather than have an inconsistent alias system. Adding ds=drawstyle is not going to make the whole alias system harder to deprecate if we really choose to go that route.
If you want to get rid of "having multiple ways to do the same thing", I'd rather first get rid of layered APIs as suggested in #11691, which are much more harmful than straight aliases IMO...

@anntzer anntzer mentioned this pull request Oct 21, 2018
Closed
@ImportanceOfBeingErnest
Copy link
Member

👍 on the separation of drawstyle and linestyle/other params. I don't know how often a construct like "steps--" is currently used; so a long enough deprecation period makes sense. Is removal == since +0.2 always?

I don't have a strong opinion on the alias.

I was wondering if it would be good to add tests for deprecations like this? I.e. a test that the intended use case (linestyle="--", drawstyle="steps") works fine (such test might exist already?) and a test that the still supported, but deprecated case throws a warning. At the end of the deprecation this should then test if the correct error is thrown. This way you would ensure not allow for this kind of mini-language API to be reintroduced in a couple of years.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 21, 2018

This way you would ensure not allow for this kind of mini-language API to be reintroduced in a couple of years.

I don't think any of the current maintainers would allow such a thing to pass review.

Our test suite is already huge enough that I wouldn't bother (there are better parts where to improve the coverage, and that doesn't necessarily mean strict numerical improvements, but rather combos of kwargs which don't work well together, see e.g. the discussion on hist/align...).

@jklymak jklymak merged commit 3d4e78b into matplotlib:master Oct 26, 2018
@QuLogic QuLogic added this to the v3.1 milestone Oct 26, 2018
@anntzer anntzer deleted the drawstyle-linestyle branch October 27, 2018 20:21
@timhoffm timhoffm mentioned this pull request May 25, 2020
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@anntzer @NelleV @timhoffm @jklymak @ImportanceOfBeingErnest @QuLogic