-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Modify hexbin
to respect :rc:patch.linewidth
#25044
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
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
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.
This seems reasonable to me. Note that the default mpl.rcParams['patch.lineiwdth'] = 1.0 so this won't be a breaking change for most people. OTOH, if someone has set to non-default this will make a (small) change in appearance. I suspect we could get away with doing this, with an API behaviour change note.
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.
Looks good to me - would you be able to put a quick note in doc/api/next_api_changes/behaviour
about this just so it's documented for users somewhere?
With a change note this should be good to go in 3.8. |
I am sorry for my long silence... Thank you so much developers for your kind suggestions. I have now added also a note in Although I was not very sure what is the best practice, I did |
I believe the failures are unrelated here. |
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.
Thanks for your work on this @yuzie007 and sorry it’s taken a while to review - I think it just slipped through the cracks. Congratulations on your first PR in Matplotlib! We hope to hear from you again.
Always thank you developpers for maintaining Matplotlib!
PR Summary
Currently in
hexbin
,linewidths
given toPolyCollection
is hard-coded as1.0
. Inpcolormesh
andhist2d
, the default value respects :rc:patch.linewidth
(default:1.0
). To improve the consistency among the methods, in this very short PR I would like to suggest respectingpatch.linewidth
also inhexbin
.Notes
While this PR does not change the default behavior, do we need to have some additional tests?
The default
linewidth
inpcolormesh
andhist2d
may be not immediately visually noticeable because the defaultedgecolors
value isnone
. (Inhexbin
,edgecolors
defaults toface
.)The default linewidths in
pcolor
andtripcolor
are also hard-coded but as0.25
. It may be also possible to respect :rc:patch.linewidth
also in these methods, while the plot looking changes whenedgecolors
other thannone
is given.In principle, it should be also possible to simply remove the
linewidths
keyword from the arguments and pass it intoPolyCollection
simply via**kwargs
, but then we need to poplinewidths
before making marginal plots and need to modify the docstring a bit also for this.I can also later add note in
doc/api/next_api_changes
if this is reasonable.PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst