Skip to content

Update definition of area in scatter examples #10930

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

Merged
merged 1 commit into from
Mar 31, 2018

Conversation

krrk
Copy link
Contributor

@krrk krrk commented Mar 31, 2018

PR Summary

Since the area or size of markers in scatter is the area of a box the two examples modified in this PR were confusing as they defined the area as the area of a circle while stating in a comment the desired range of radii. With the previous definition of area the markers would only have a maximum of pi/4 times the desired area (sqrt(pi/4) times the desired radius). The new definition of area now agrees with their respective comments.

Perhaps to even better agree with the stated radii in the comments the edge patch should be disabled with edgecolors='none' in the call to scatter as the edge patch adds about 2 pixels to the radius.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@tacaswell tacaswell added this to the v2.2-doc milestone Mar 31, 2018
@tacaswell tacaswell merged commit 0b0a6a0 into matplotlib:master Mar 31, 2018
@tacaswell
Copy link
Member

Thanks @krrk !

Congratulations on your first contribution to Matplotlib 🎉 Hopefully we will hear from you again.

jklymak added a commit that referenced this pull request Apr 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants