-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Deprecate parameter orientation of bar() and barh() #17322
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
Can't this be done by just using _rename_parameter on bar()? (the parameter is already keyword-only)
It's more a deprecation than a rename. And _rename_parameter does not allow to set a message. It would recommend using _orientation, which we certainly do not want.
4c3cc9d to
a40a9e9
Compare
fair enough
I guess the question, though, remains as to why this should be private. For example we don't have hist() and histh(), just hist(..., orientation=...). It is true that the argument names are not optimal if using the "wrong" orientation, but I'm not sure that's a strong reason for the removal.
I guess I'm +/-0 on the removal :)
I guess the question, though, remains as to why this should be private.
My main motivation is that https://matplotlib.org/devdocs/api/_as_gen/matplotlib.axes.Axes.bar.html should not document orientation as "This is for internal use only." I don't want to have parameters, which we tell people to not use.
If we would design this from scatch, we could use direction-agnostic parameter names. But as is stands, the parameter of bar(..., orientation='horizontal') is rather confusing. Also, there's axh/vlines and axh/vspan. So there's other precedent of using two separate functions instead of orientation.
I think there should be one official way to draw horizontal bars. Other possibilites would be to deprecate barh() or to just remove the docs on orientation. But I think, hiding orientation is the least invasive change.
I think undocumenting it is good enough. (I'm not blocking the rename, just not so convinced by it.)
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.
if we are going to rename we should add a check that we don't get both.
I am 👍 on un-documenting it, 50/50 on renaming it.
It is already not in the signature and keyword-only (as we are doing kwargs popping) so the upside of renaming is relatively small (it is already hidden from auto-completers) and we can get it out of the docs by the text describing it from the docstring to a in-line comment. On the other hand, we would make some users change there code to remove some thing that has been documented more-or-less forever (looks like the doc and the functionality came in via f0be66e but was only labeled as "for internal use" in e078f91 for 2.1).
Can we push this to 3.4?
I've extracted the un-documentation part to #17504. We can decide for 3.4 if we want to deprecate the parameter in favor of a private version.
I'm not following up on this. The undocumented state is good enough.
PR Summary
While stated to be internal, there's a parameter 'orientation' to
bar(), which is also documented. To make this very clear, this PR deprecates 'orientation' and uses a private '_orientation' parameter instead.The aim is to finally remove 'orientation' also from the docs, because it's rather confusing than helpful to document a parameter and say it's not to be used.