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

Bokeh, transform all colors to hex #956

Closed
ahartikainen opened this issue Dec 8, 2019 · 27 comments · Fixed by #1084
Closed

Bokeh, transform all colors to hex #956

ahartikainen opened this issue Dec 8, 2019 · 27 comments · Fixed by #1084
Assignees
Labels
Beginner Enhancement Improvements to ArviZ

Comments

@ahartikainen
Copy link
Contributor

ahartikainen commented Dec 8, 2019

Input all colors to bokeh functions as hex.

We can use matplotlib helper functions to transform special syntax

color="C0"

etc

@OriolAbril OriolAbril added Enhancement Improvements to ArviZ Good first issue labels Jan 5, 2020
@mrinalnilotpal
Copy link

@OriolAbril can I try fixing it?

@OriolAbril
Copy link
Member

@mrinalnilotpal Definitely!

@animesh-007
Copy link

can anybody tell me which files I should look at in order to solve the issue?

@ahartikainen
Copy link
Contributor Author

I think you would need to create a function that transforms certain colors (rgb, 'C{i}' syntax etc to hex)

These are for bokeh plots

@aloctavodia
Copy link
Contributor

aloctavodia commented Jan 21, 2020

matplotlib have some functions for color conversion. https://matplotlib.org/api/colors_api.html#functions and the "CN" colors can be accessed from matplotlib.rcParams['axes.prop_cycle']

@sahajsk21
Copy link

Is this issue still to be resolved?

@OriolAbril
Copy link
Member

@mrinalnilotpal @animesh-007 @sahajsk21 It is still pending. Just to make sure it is clear what this PR entails, I'll try to extend the description. The goal is to have some easy way to convert all kinds of colors used by matplotlib (such as C0, C1 to refer to default colors in prop_cycle, named colors, single letter color abbreviations, rgb...) to hex in order to use them in bokeh. Then, modify the code where necessary to make both backends plot the same colors (which is currently not the case, e.g. matplotlib's ppc vs bokeh's ppc

It looks like to_hex is exactly what we are looking for. I think a way to tackle this would be to convert all color references to hex before passing them to the backend files. Then both backends should plot the same colors without much hassle.

@sahajsk21
Copy link

Thanks @OriolAbril I'll be working to solve this issue

@amukh18
Copy link
Contributor

amukh18 commented Feb 18, 2020

@OriolAbril I think this issue has been inactive for a while. Could I work on it?

@OriolAbril
Copy link
Member

@sahajsk21 do you still plan to work on this?

@sahajsk21
Copy link

@amukh18 @OriolAbril I have been working on issue so he can take up the issue.Thanks

@OriolAbril
Copy link
Member

I have been working on issue so he can't take up the issue.Thanks

Could there be a typo so the message is this? @sahajsk21 There is no rush, work at your own pace, this is why we ask :).

Sorry if my previous wording sounded harsh, it wasn't the intention. Also, please let us know if you were to need any help

@sahajsk21
Copy link

@OriolAbril No it wasn't a typo. Don't worry, I didn't take the comment in the negative sense. Actually, I have been going through the code and making myself familiar first before working on the issue. That is why it is taking so long.
But I think @amukh18 can still take up this issue.

@amukh18 @OriolAbril I have been working on another issue so he can take up the issue. Thanks

@amukh18
Copy link
Contributor

amukh18 commented Feb 19, 2020

@sahajsk21 @OriolAbril Thank you. I'll begin working on it right away.

@amukh18
Copy link
Contributor

amukh18 commented Feb 21, 2020

@OriolAbril I have a question. If I were to add the to_hex function to plot_khat as follows:

if hlines_kwargs is None:
        hlines_kwargs = {}
    hlines_kwargs.setdefault("linestyle", [":", "-.", "--", "-"])
    hlines_kwargs.setdefault("alpha", 0.7)
    hlines_kwargs.setdefault("zorder", -1)
    hlines_kwargs.setdefault("color", to_hex("C1"))

    if coords is None:
        coords = {}

    if color is None:
        color = "C0"
    color = to_hex(color)

Is this supposed to return any errors?

@OriolAbril
Copy link
Member

I think this should work for this case

@amukh18
Copy link
Contributor

amukh18 commented Feb 21, 2020

Hmm, pytest returned some errors. I will see what the unit tests on the pipeline say on committing.

@OriolAbril
Copy link
Member

There is now a converter function available (added by #1084), however, is is not used in all plots yet. I am reopening the issue as a remainder to use the converter in all plots

@OriolAbril OriolAbril reopened this May 10, 2020
@OriolAbril
Copy link
Member

OriolAbril commented Jun 17, 2020

plot_hdi is a great candidate to take advantage of this: bokeh code already uses matplotlib cycle:

EDIT: I updated the setting of defaults in #1241 to reduce these lines below:

color = plot_kwargs.pop("color")
if len(color) == 2 and color[0] == "C":
color = [
prop
for _, prop in zip(
range(int(color[1:])), cycle(mpl_rcParams["axes.prop_cycle"].by_key()["color"])
)
][-1]
plot_kwargs.setdefault("line_color", color)

To a single line when setting the defaults in plot_kwargs and fill_kwargs


Plenty (if not all) functions could also take advantage of this which will give both matplotlib and bokeh plots the same style and colors 🎨

@OriolAbril OriolAbril added Beginner Enhancement Improvements to ArviZ and removed Enhancement Improvements to ArviZ Good first issue labels Jul 13, 2020
@Rishabh261998
Copy link
Contributor

@OriolAbril would like to work on this. Thanks

@OriolAbril
Copy link
Member

I have assigned it to you. I have to confess I have not followed this for a while so I don't really know which plots use this and which don't, you'll have to first check where it's needed and then make the changes.

@ch1booze
Copy link

ch1booze commented Oct 3, 2022

@OriolAbril Is this issue still open?

@ch1booze
Copy link

ch1booze commented Oct 3, 2022

If so, I would like to be assigned the task. Pending your reply, I am getting acquainted with the codebase.

@ch1booze
Copy link

ch1booze commented Oct 3, 2022

@OriolAbril Upon cloning and reviewing the code, it seems as if the refactoring needed has been done by @Rishabh261998.
I checked if he made any PRs regarding this issue, but I didn't see any or maybe I am looking at the whole PR thing wrong.
I am still in search of my first Open Source contribution😋.

@Dhruv3sood
Copy link

import itertools
import matplotlib.pyplot as plt

def process_color(color, plot_kwargs):
"""Process color input and set line color in plot_kwargs."""
if color[0] == "C":
# Use color cycle from matplotlib
color_cycle = itertools.cycle(plt.rcParams["axes.prop_cycle"].by_key()["color"])
line_color = next(itertools.islice(color_cycle, int(color[1:]), None))
else:
line_color = color
plot_kwargs.setdefault("line_color", line_color)

Alternative updated code for the commit above

color = plot_kwargs.pop("color")
if len(color) == 2 and color[0] == "C":
color = [
prop
for _, prop in zip(
range(int(color[1:])), cycle(mpl_rcParams["axes.prop_cycle"].by_key()["color"])
)
][-1]
plot_kwargs.setdefault("line_color", color)

@jasochen7
Copy link

Is this issue still open?

@OriolAbril
Copy link
Member

It should have been closed already @jasochen7, closing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Beginner Enhancement Improvements to ArviZ
Projects
None yet
Development

Successfully merging a pull request may close this issue.