Skip to content

pyreverse: use colorblind friendly default colors #8415

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

Conversation

DudeNr33
Copy link
Collaborator

@DudeNr33 DudeNr33 commented Mar 9, 2023

Type of Changes

Type
✨ New feature

Description

As discussed in the issue, with 3.0 we can now change the default color palette.

Closes #8251

@DudeNr33 DudeNr33 added pyreverse Related to pyreverse component Breaking changes for 3.0 🦤 labels Mar 9, 2023
@DudeNr33 DudeNr33 added this to the 3.0.0b1 milestone Mar 9, 2023
@DudeNr33 DudeNr33 requested a review from nickdrozd March 9, 2023 17:55
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I did not review the graphical result... Code's LGTM 😄

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Mar 9, 2023

For reference, this is how the new colors would look like:

image

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #8415 (347c76a) into main (62889d4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8415      +/-   ##
==========================================
+ Coverage   95.68%   95.73%   +0.04%     
==========================================
  Files         175      174       -1     
  Lines       18465    18392      -73     
==========================================
- Hits        17668    17607      -61     
+ Misses        797      785      -12     
Impacted Files Coverage Δ
pylint/pyreverse/main.py 93.87% <ø> (ø)
pylint/pyreverse/plantuml_printer.py 100.00% <100.00%> (ø)

... and 14 files with indirect coverage changes

@Pierre-Sassoulas
Copy link
Member

It looks fine with protanopia (no red) but it's a little hard with other form of colorblindness.

No blue:

tritanopia (no blue)

No coolor:
achromatopia (no color at all)

No green:
deuteranopia (no green)

orange 2 and goldenrod 9 are not great in particular. This is not something that can be improvised. What were the color before that (maybe it's already taken into account by plantuml) ? Otherwise some research are in order I think :D

@github-actions

This comment has been minimized.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Mar 9, 2023

Oh that's cool, what tool did you use to make the different versions? Or did you just desaturate it yourself?

I tried to adapt the colorblind palette from seaborn with the closest matching HTML colors.
But I will try to find something better. 👍

@Pierre-Sassoulas
Copy link
Member

It's firefox dev tools for accessibility (ctrl + shift + I), but there's probably the same thing in chrome. If it's already a colorblind palette then I don't think we're going to do much better than that, this is a hard problem to solve. They probably made the better result for red color blind because it's the most prevalent, it can't be perfect.

@DanielNoord
Copy link
Collaborator

I am actually colorblind and these colours are indeed sub-optimal. Funilly enough the "no green" output Pierre gives works really well for my type of colour blindness but I don't know if it would work for others.

@Pierre-Sassoulas
Copy link
Member

I've found three possible color palettes:
IBM's : https://lospec.com/palette-list/ibm-color-blind-safe (It's IBM but there's only six of them)
Paul Tol's : https://personal.sron.nl/~pault/ (7, up to 10)
Ban wong: https://www.nature.com/articles/nmeth.1618 (8)

What's the best one in your opinion @DanielNoord ? Also do you know what type of color blindness you have ? It seems we should target red/green rather than blue/yellow, because red/green is a lot more common.

@DanielNoord
Copy link
Collaborator

I have red/green, the second link works quite well for me!

@Pierre-Sassoulas
Copy link
Member

All figures in the second one, or a particular one ? :)

@DanielNoord
Copy link
Collaborator

All figures in the second one, or a particular one ? :)

All are quite good, but figure 1 is definitely the best!

@Pierre-Sassoulas
Copy link
Member

I think we should use figure 7 then, there's 9 different values and the contrast with the black text is still high enough (compared to say the dark blue in figure 4 where there is more values but I can't read the value in it personally).

fig7

Let me know if you want me to translate that in plantuml color @DudeNr33 :)

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

(Colorblindness to take into account)

@DudeNr33
Copy link
Collaborator Author

Thank you both for your input and research! The developer tools option is extremely helpful, and having feedback from Daniel as well! 👍
I will update the default colors accordingly.

@DudeNr33
Copy link
Collaborator Author

I used the hex codes directly and made the PlantUMLPrinter work with both hex codes and CSS names.

Here is an example:
image

Updated the functional test file to make sure that both CSS names and HEX values can be given through the command line.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Amazing we even got an enhancement for hexcode !

"#EEDD88", # light yellow
"#EE8866", # orange
"#FFAABB", # pink
"#DDDDDD", # pale greay
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"#DDDDDD", # pale greay
"#DDDDDD", # pale grey

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oopsie

"gold",
"hotpink",
"mediumspringgreen",
"#77AADD", # light blue
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"#77AADD", # light blue
# colorblind friendly palette taken from... (sorry I'm on mobile)
"#77AADD", # light blue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it definitely makes sense to not only put the link in the news fragment, thanks for the catch!

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

👌

@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) March 10, 2023 18:55
@Pierre-Sassoulas Pierre-Sassoulas changed the title pyreverse: replace default colors pyreverse: use colorblind friendly default colors Mar 10, 2023
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 0d26e3c into pylint-dev:main Mar 10, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0b1, 3.0.0a6 Mar 10, 2023
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 347c76a

@DudeNr33 DudeNr33 deleted the pyreverse-replace-default-colors branch March 11, 2023 08:53
@nickdrozd
Copy link
Contributor

I just saw the new colors -- they look great! 🎨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking changes for 3.0 🦤 pyreverse Related to pyreverse component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use better default colors for pyreverse
4 participants