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

added random colormap option to cm.py (lines 705-740ish) #24396

Closed

Conversation

augustvanhout
Copy link

PR Summary

Original PR #24340
Matplotlib has a huge number of colormaps and no way to quickly sample them! The above PR's comments contain spirited debate and a video of the proposed functionality - passing the argument 'cmap = "random"'

This PR adds a few lines to cm.py to allow coders everywhere to sample matplotlib's many colormaps on any plot they like, anytime they like.

Two things are changed in cm.py:
1) added one-liner function get_random_cmap() which returns a Colormap at random from _colormaps
2) added if statement to _ensure_cmap so that when a user passes "random" as a colormap, _ensure_cmap returns get_random_cmap()

And that's it.

The workflow was:
- Edit cmp.y and test functionality in venv
- Ensure flake8
- Run pytest - 1 depreciation warning when pickling
- Edit docstrings - went with precedent of other similarly short functions in cm.py
- Did not edit whats_new_next as feature is small

Attached is an example dataset (credit to Kaggle) as well as a script called random_colormap_test.py, which showcases the changes proposed.

melbourne.csv
random_colormap_test.txt

PR Checklist

Tests and Styling

  • [✓] Has pytest style unit tests (and pytest passes).
  • [✓] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [NA] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/

  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/

  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@story645
Copy link
Member

story645 commented Nov 8, 2022

Gonna reiterate what I said in #24340 (comment) that I think this could work as part of an example in the colors section of the gallery given we don't really have standalone examples of using the new registry dictionary.

While I like your video and it's on my to-do list to share to all the places, I still really don't think this works as a colormap option. Like folks said in #24340, 'random' indicates the name of a colormap, so using it to trigger a random colormap creates an inconsistency in the API. We had a really similar discussion around using 'None' to get viridis and settled on a new method in #24111 to keep consistency. Not that I think you should add a random method to a dictionary like object - like with 'random' that's not really what it's for - the colormap registry object exists to store colormaps in an easily retrievable deterministic way.

Eta: sorry should have read the PR a bit closer, but I'm still not sold that the maintenance cost of this being in the library is worth it. Like if we take it, and currently there's no support for it so please don't do anymore work, it would need the what's new and the version added and tests and all that overhead cause that's how stuff works here. I think for the person this is for, they might honestly probably be more likely to discover it in a gallery example than buried in the colormap API anyway.

@jklymak
Copy link
Member

jklymak commented Nov 8, 2022

@augustvanhout thanks so much for your enthusiasm! However, maybe the level of consensus wasn't clear in #24340 because only two maintainers directly engaged. I am also strongly against this. I think if anyone disagreed with @timhoffm about closing the PR, we would have said. Hopefully we hear from you again, perhaps with a cool example as suggested by @story645

@augustvanhout
Copy link
Author

augustvanhout commented Nov 8, 2022 via email

@story645
Copy link
Member

story645 commented Nov 8, 2022

I’ll find some bugs to stomp… and maybe someday invent something more nuanced!

Turning this PR into a documentation PR is mostly the same code you used to make your video, but placed in https://github.com/matplotlib/matplotlib/tree/main/examples/color

@jklymak agreed that the example idea could work and for most docs you need one maintainer in agreement (here you've kinda got two) and nobody in opposition so far.

@tacaswell
Copy link
Member

I agree with @jklymak and @story645 . This will likely be good as an example, but is not something we want to ship as a function in the core library. It is better to document how to use the standard library function tools to do this and special casing the string "random" to mean "pick a random color map of the registered ones" not "a color map named 'random' which has been registered" is a big conceptual change.

Thank you again for you work and enthusiasm, but I am going to close this PR. If you are still interested in working on this please follow up with a documentation PR as both Hannah and Jody have suggested.

@tacaswell tacaswell closed this Nov 8, 2022
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.

None yet

4 participants