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

Add Tol palettes for accessibility #11914

Merged
merged 8 commits into from
Dec 31, 2021

Conversation

karen-poon
Copy link
Member

This PR is to add Paul Tol's palettes which can make Bokeh more accessible for people with colour vision deficiencies. This is also to follow up the discussion in the previously closed PR #11887.

Naming and Existing Palette

  • I have renamed the section as "Accessible Palettes".
  • I preserved the existing palette Colorblind.
  • I have created 2 new dictionaries in palettes.py, which are tol and accessible. The accessible dictionary would include both tol and Colorblind, while tol only includes Tol's palettes.
  • Also added the new palettes to bokehjs/src/lib/api/palettes.ts.
  • Since Brewer already has PRGn and YlOrBr, I have added a prefix to Tol’s: TolPRGn and TolYlOrBr. It is worth noting that Brewer and Tol uses slightly different colours for these 2 palettes, so this is the way we can distinguish them.
  • As discussed before, since Colorcet already has rainbow, I have named Tol's as TolRainbow for now.

Documentation

  • I have added Tol's BSD license statement in both palettes.py and palettes.ts.
  • Other than renaming the section as "Accessible Palettes", I have also linked Tol's website (https://personal.sron.nl/~pault/) and where Colorblind came from (https://jfly.uni-koeln.de/color/#pallet) to the page.
  • I have added tol in the "Other Attributes" section so that people can know about such attribute's existence.

Examples

I have updated the following examples into using the new accessible palettes (and those that appeared in the user guide). I have also updated the images of those appeared in the Gallery.

  • bar_colormapped.py
  • bar_colors.py
  • eclipse.py
  • image.py
  • stacked_area.py

Overview of the new palettes
image image image image image image image image image image image image
image image

*Quite a huge PR but please let me know if I have mistyped any colour codes or anything should be modified/added. Thank you!

@bryevdv
Copy link
Member

bryevdv commented Dec 29, 2021

Hi @karen-poon thanks for your patience during busy end-of-year times. This PR looks fantastic. I only have a couple of comments/questions off the bat:

Pale/Dark palettes

The Tol site has this to say about the Pale/Dark palettes:

The bright, high-contrast, vibrant, muted and medium-contrast schemes work well for plot lines and map regions, but the colours are too strong to use for backgrounds to mark (black) text, typically in a table. For that purpose, the pale scheme is designed (Fig. 6, top). The colours are inherently not very distinct from each other, but they are clear in a white area. The dark scheme (Fig. 6, bottom) is meant for text itself on a white background, for example to mark a large block of text. The idea is to use one dark colour for support, not all combined and not for just one word.

By my understanding, these "palettes" are intended to be used by picking one of the few colors as a good text color (depending on light/dark background). This is opposed to how most palettes are used, which is to drive a colormapping that uses the entire palette. (I think the key difference of these palettes is captured in the sentence above "The colours are inherently not very distinct from each other", that definitely different from all the other palettes for colormapping)

What do you think about renaming these two to PaleText and DarkText to emphasize this intended usage? I would also consider removing them from the accessible dictionary and having them be "standalone" but input on that idea is welcome.

stacked area example

I have to be honest and the new stacked area PNG is pretty harsh to look at with that many layers of the rainbow palette stacked. What do you think about switching the palette to Muted or Sunset and/or reducing the number of stacked areas down to three or four?

Naming

cc @tcmetzger I think in the issue it was discussed to have the dictionary be named colorblind instead of accessible. But then we have to figure out what to do about the old "Colorblind" palette. Thoughts here are welcome!

"Colorblind" : Colorblind
tol = {
"Bright": Bright,
"HighContrast": cast(Dict[int, Any], HighContrast),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this cast should be necessary, but I think it might be possible to fix this by explicitly specifying a type where HighContrast is defined, rather than a cast here (and I'd regard that as marginally better)

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the cast, mypy raises this error when I add HighContrast to dictionaries like accessible or all_palettes:

incompatible type "Dict[str, object]"; expected "Mapping[str, Dict[int, Tuple[str, ...]]]"

Mypy infers the type based on the first assignment it reads. We defined Bright = { 3: Bright3, 4: Bright4, 5: Bright5, 6: Bright6, 7: Bright7 }, which has a type of Dict[int, Tuple[str, ...]]. However, for HighContrast = { 3: HighContrast3 }, it has a type of Dict[int, Tuple[str, str, str]], which mypy thinks they are different. So when we add HighContrast to accessible and all_palettes, it became Dict[str, object] instead of Mapping[str, Dict[int, Tuple[str, ...]]].

The only way I can think of solving this is by casting... I can cast the original HighContrast with Dict[int, Tuple[str, ...]] instead of Any if that's preferred.? I'm a bit new to python too so it will be greatly appreciated if you could guide me on how to exactly solve this 🙏

Copy link
Member

Choose a reason for hiding this comment

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

I was suggesting something like this (but, I have not actually tried it)

HighContrast: Dict[int, Tuple[str, ...]] = { 3: HighContrast3 }

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I think it's working :)

export const Bright4 = [0x4477aaff, 0xee6677ff, 0x228833ff, 0xccbb44ff]
export const Bright5 = [0x4477aaff, 0xee6677ff, 0x228833ff, 0xccbb44ff, 0x66cceeff]
export const Bright6 = [0x4477aaff, 0xee6677ff, 0x228833ff, 0xccbb44ff, 0x66cceeff, 0xaa3377ff]
export const Bright7 = [0x4477aaff, 0xee6677ff, 0x228833ff, 0xccbb44ff, 0x66cceeff, 0xaa3377ff, 0xbbbbbbff]
Copy link
Contributor

Choose a reason for hiding this comment

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

In cases where the next palette only appends a value, one can use the following to reduce duplication and JS size:

Suggested change
export const Bright7 = [0x4477aaff, 0xee6677ff, 0x228833ff, 0xccbb44ff, 0x66cceeff, 0xaa3377ff, 0xbbbbbbff]
export const Bright7 = [...Bright6, 0xbbbbbbff]

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. Should I apply this to all the other palettes? I saw some previously defined colours were also written with duplications.

Copy link
Member

Choose a reason for hiding this comment

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

It would definitely be a good thing to apply this pattern everywhere that it can be applied

@mattpap mattpap added this to the 3.0 milestone Dec 29, 2021
@karen-poon
Copy link
Member Author

Thank you so much for reviewing my PR! Here are my thoughts:

  1. For the Pale/Dark palettes, I think DarkText sounds like a good name for its purpose; but for Pale, since the doc mentions that we should "use the pale colours for the background of black text", should we name it as something like PaleBackground instead?
  2. Should we still define Pale/Dark with the 3, 4, 5... if it's not used for colormapping?
  3. I can definitly change the palette for the stacked area example :)
  4. I personally think if we renamed the section as Accessible Palettes, it would be better to use the word accessible as a dictionary name instead of colorblind for consistency. But I also like the idea of not having the accessible dictionary and just leave with the tol dictionary so we can have a Tol collection of palettes.

@bryevdv
Copy link
Member

bryevdv commented Dec 30, 2021

@karen-poon Good questions!

  1. PaleBackground or PaleTextBackground seem ok (the latter is a bit more specific)

  2. Good point, I don' t think we need them, i.e. just one DarkText with all the colors seems fine

  3. I'm 👍 for your suggestion, we can always revisit a final pass over things before release too

@karen-poon
Copy link
Member Author

@bryevdv I have performed the following updates:

  • Renamed Pale/Dark as PaleTextBackground and DarkText respectively (liked PaleTextBackground more because it shows that it's a background of a text).
  • Removed the accessible dictionary.
  • Removed Pale/Dark from the tol dictionary as well because without the 3,4,5... they are not of the same type anymore. But I still think that having a collection of Tol palettes is better and it still makes sense to separate Pale/Dark because they are of different usage anyways. Suggestions are welcome tho :)
  • Updated documentation regarding the usage of Dark/Pale.
  • stacked_area has been changed to use the Sunset palette.

Comments:

  • I will reduce the duplications in the palettes.ts file once I have more time.
  • I'm not sure why my js-test keeps on failing in the CI. The js/bokeh-api.min.js was checked when I tried to run it locally. (My local result is 108 kB <= 120 kB but CI gives 123 kB > 120 kB)

@bryevdv
Copy link
Member

bryevdv commented Dec 31, 2021

I will reduce the duplications in the palettes.ts file once I have more time.
I'm not sure why my js-test keeps on failing in the CI. The js/bokeh-api.min.js was checked when I tried to run it locally. (My local result is 108 kB <= 120 kB but CI gives 123 kB > 120 kB)

I expect sizes may differ slightly on different platforms, I would say let's see how things are once you can reduce the duplications, that might just solve things. If not we can bump the limit as needed.

@mattpap
Copy link
Contributor

mattpap commented Dec 31, 2021

Unless there is anything more to address here, let's resolve code duplication in another PR.

@bryevdv
Copy link
Member

bryevdv commented Dec 31, 2021

Thanks @karen-poon !

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

3 participants