-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Prefer plot() to scatter() in examples. #14174
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
@@ -35,7 +35,7 @@ | |||
pivot='mid', units='inches') | |||
qk = ax2.quiverkey(Q, 0.9, 0.9, 1, r'$1 \frac{m}{s}$', labelpos='E', | |||
coordinates='figure') | |||
ax2.scatter(X[::3, ::3], Y[::3, ::3], color='r', s=5) | |||
ax2.plot(X[::3, ::3], Y[::3, ::3], 'r.', ms=3) |
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.
I'd prefer to keep the explicit kwargs (ie. color='r', marker='.'
) as then it's obvious what is happening - it's definitely not intuitive what 'r.'
means.
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.
switched to ".", c="r"
(unfortunately marker="."
is not sufficient as one also need to pass linestyle="none"
, which becomes quite a mouthful).
Therefore the note in scatter
does not sufficiently cover the topic, e.g.
I have no clear idea how to accomodate both aspects, but we should not just blindly follow the technical argument. |
There's actually another reason to prefer plot() to scatter() when variable markers are not needed (but I didn't want to get into the details here): scatter autoscaling on log scale is poor (#3059, #5897, #10782, #11898), and one can easily hit that e.g. when changing the scales interactively. It is possible that #13642 will improve the situation, but it's not merged yet. |
Marker size is a style choice, it really depends on your application. You may be right with log scaling. However, in the end, this supports my point: There's more difference between I'm open which one to use under which circumstances, but that needs clear documentation and instructions. Without that e.g. using |
I can just revert the two "scatter hist" examples for now? None of the other examples mention the term "scatter", and I have no doubt there's a bunch of other places in the examples where we use |
The "Scatter hist" are the two most obvious scatter-like ones. The only clear case in which I really prefer I won't give a positive review until we've documented the aspects discussed above #14174 (comment). But if you at least leave out "Scatter hist", I'm not blocking this if others choose it's ok. |
Reverted the scatter hist examples. |
... when markers don't vary in size or shape. The docstring of scatter() suggests to do so (for performance), so let's showcase it.
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
Let's not bother. |
... when markers don't vary in size or shape.
The docstring of scatter() suggests to do so (for performance), so let's
showcase it.
PR Summary
PR Checklist