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

Adding glasbey colormaps #25

Merged
merged 38 commits into from
Apr 1, 2019
Merged

Adding glasbey colormaps #25

merged 38 commits into from
Apr 1, 2019

Conversation

jsignell
Copy link
Member

@jsignell jsignell commented Mar 20, 2019

Adding glasbey colors according to a new naming pattern with aliases for Category, Category10, and Category20.

To Do:

  • add assets
  • add to module
  • write tests
  • write up docs
  • add an example

Closes: #11

@jsignell jsignell changed the title [WIP] Adding glasbey colormaps Adding glasbey colormaps Mar 20, 2019
@jsignell jsignell requested a review from jbednar March 20, 2019 18:54
@jsignell jsignell self-assigned this Mar 20, 2019
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Thanks for hitting this home! I have a few suggestions below, plus:

  • I think we don't need to explicitly state "no_black" in any of the names, because all of them are "no_black" if I followed it correctly.
  • Are you using the modified glasbey.py and passing --min_chroma=10? I think that's important, or else there are a lot of gray colors included. If not, maybe add those and call them no_gray, and make those the default versions for which to use the short names.

After that I'm ready to merge and start using these!

assets/CET_to_py.py Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
examples/assets/write_named.py Outdated Show resolved Hide resolved
examples/index.ipynb Outdated Show resolved Hide resolved
examples/user_guide/Glasbey_Colors.ipynb Outdated Show resolved Hide resolved
"cell_type": "markdown",
"metadata": {},
"source": [
"In addition to these long form names, colorcet provides several aliases to the most useful of the glasbey colormaps. These include - `glasbey`, `Category10`, and `Category20`. `Category10` and `Category20` can be used as replacements for the bokeh colormaps of the same name since they start with the same sequence."
Copy link
Member

Choose a reason for hiding this comment

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

I have gotten feedback from colorcet users that using the same name as any common colormap from a different package causes confusion, so I think Category10 should be maybe gCategory10, and similarly for Category20. Yes, these colormaps are in a different namespace, but people have still had trouble...

@jsignell jsignell dismissed jbednar’s stale review March 26, 2019 20:04

Made requested changes

@jsignell
Copy link
Member Author

I think this is ready to go in @jbednar

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Looks good. About to push changes to Glasbey_colors.ipynb, and then it's back over to you. I might rename Glasbey_Colors to Categorical.ipynb, maybe? And then the main User Guide index needs to be updated to point to it and remove the categorical ones from its lists.

assets/CET_to_py.py Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
colorcet/tests/test_matplotlib.py Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Looks good. Probably should rename Glasbey_Colors.ipynb to Categorical.ipynb, and index.ipynb to Continuous.ipynb.

examples/user_guide/Glasbey_Colors.ipynb Outdated Show resolved Hide resolved
assets/CET_to_py.py Outdated Show resolved Hide resolved
@jbednar
Copy link
Member

jbednar commented Mar 29, 2019

Also, should the code bits be moved to a new file in colorcet, imported from __init__.py? Having to maintain them as strings is awkward.

swatch(*arg, **kwargs) for
arg in args]).cols(cols)

backends = hv.Store.loaded_backends()
Copy link
Member

Choose a reason for hiding this comment

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

@jlstevens , it would be nice if backends not loaded would simply not validate, to avoid having to have code like this that checks the loaded_backends. Maybe something that can go into this hv release?

@jsignell jsignell dismissed jbednar’s stale review March 29, 2019 17:23

Restructured to put continuous and categorical next to each other

assets/CET_to_py.py Outdated Show resolved Hide resolved
@jsignell
Copy link
Member Author

I built the website locally and it looks fine.

@jsignell
Copy link
Member Author

jsignell commented Apr 1, 2019

Good to merge?

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Looks great! Happy to merge after the suggestion is applied.

examples/user_guide/index.ipynb Outdated Show resolved Hide resolved
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