Skip to content
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

Remove check for sizes=0 in legend_elements #20269

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented May 20, 2021

PR Summary

This check removed legend labels that have sizes close to 0 in the legend. So now that I removed it, everything that's in the plot will always be shown in the legend labels as well which makes sense to me and is easy to test.

s=0 values are still in the plot being calculated with and slowing the down the plot. So using s=0 to remove "bad" points from plot and legend seems like a bad habit that doesn't need encouragement. If you want to filter it should've been done before creating the scatter like:

x = x[np.isclose(s, 0)]
y = y[np.isclose(s, 0)]
s = s[np.isclose(s, 0)]
ax.scatter(x, y, size=s)

One could also argue that we are just fixing a bug because:

  • It's not documented in the docs.
  • It's not tested for.
  • Users can't change the np.isclose() parameters, so maybe there's some user out there working with small numbers and needs to change the parameters. So it's better to just let the user filter out points close to 0 before plotting the data.

PR Checklist

  • Requires Allow the func argument in PathCollections.legend_elements to return strings #19556
  • Has pytest style unit tests (and pytest passes).
  • [N/A] Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

ambiguous variable name 'l'
Remove repeated ifs, duplicate variables, multiple min/max calls.
no need to for set_locs in for loop
Removing 0 sized markers in the legend is a little weird. Feels more like a user error to add 0 sized markers to the plot. Maybe it should've started to error when adding that to the scatter?

0 in sizes broke the tests, as well as changing seeds. Still not perfect because num=9 isn't always respected.
Add and improve some comments.
Rename variables for better readabillity.
Use LinearLocator for to get evenly distributed indexes. It's more inline with the numerics and allows more flexibility.
@Illviljan Illviljan marked this pull request as draft May 20, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant