Skip to content

Speedup LinearSegmentedColormap.from_list. #19287

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
Jan 18, 2021
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 13, 2021

... by special-casing lists-of-rgb(a) tuples in to_rgba_array.

This speeds up colorcet's import nearly twofold, from ~430ms to ~250ms
on my machine. (colorcet generates a lot of colormaps at import time.)

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • 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).
  • 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).

@@ -362,10 +362,16 @@ def to_rgba_array(c, alpha=None):
if len(c) == 0:
return np.zeros((0, 4), float)
else:
if np.iterable(alpha):
return np.array([to_rgba(cc, aa) for cc, aa in zip(c, alpha)])
lens = {len(cc) if isinstance(cc, (list, tuple)) else -1 for cc in c}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if one of them is already a NumPy array? I guess it will just take the slow path, then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it will, but that's quite artificial. Users would have to provide something like

[np.array(r1, g1, b1),
 np.array(r2, g2, b2),
 ...
]

@anntzer This should get a comment explaining that all branches do the same and the distinction is for performance optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QuLogic yes, @timhoffm's comment about artificiality applies.
@timhoffm sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I'd consider artificial necessarily. If someone called to_rgba_array (perhaps indirectly via a getter on some artist) and then passed individual colours from it, then it could be an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, OTOH adding np.ndarray to the tuple of instances checked would complicate the code a bit as you can't necessarily call len on a ndarray (it may be zero-dim). There's actually a test that exercises that case...
For now I guess falling back on the slow path is just fine for that case.

... by special-casing lists-of-rgb(a) tuples in to_rgba_array.

This speeds up colorcet's import nearly twofold, from ~430ms to ~250ms
on my machine.  (colorcet generates a lot of colormaps at import time.)
@timhoffm
Copy link
Member

@QuLogic is there a reason you did not merge after the second positive review?

@QuLogic
Copy link
Member

QuLogic commented Jan 18, 2021

CI fixes had not been confirmed at the time.

@QuLogic QuLogic merged commit 30b5b24 into matplotlib:master Jan 18, 2021
@QuLogic QuLogic added this to the v3.4.0 milestone Jan 18, 2021
@anntzer anntzer deleted the fastlcm branch January 18, 2021 22:22
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.

3 participants