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

Updated the code to include support for matplotlib colormaps #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pakitochus
Copy link

By default it uses colormap='rainbow' to generate colors, so that there is no error if len(x)>10. A list of color can still be specified, and if len(x) not > 10 it will apply. hex2rgb has changed to numpy array to comply with matplotlib colormaps specification.

Copy link
Owner

@fengwangPhysics fengwangPhysics left a comment

Choose a reason for hiding this comment

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

@pakitochus Thanks for your interest in the code for chord diagram. I have a few comments on your changes, please check and update accordingly. Thanks.

if (len(x) > 10) or (colors is None):
cm = plt.get_cmap(cmap)
colors = cm(np.linspace(0,1,len(x)))[:,:3]
else:
Copy link
Owner

Choose a reason for hiding this comment

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

This "else" will apply only when len(x)<=10 and (colors is not None). The colors will be changed in the following codes, hence this seems to be a bug.

My suggestion:

if colors is None:
    if (len(x) > 10) or (cmap is not None):
        cm = plt.get_cmap(cmap)
        colors = cm(np.linspace(0,1,len(x)))[:,:3]
    else:
        # use default d3.js category10

Copy link
Author

@pakitochus pakitochus Jan 26, 2018

Choose a reason for hiding this comment

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

In this case, it is when if (len(x) > 10) or (colors is None):, implying or, not and. I mean, if any of these two conditions are met, we use the default colormap. I don't know if this is what you meant. Your suggestion would have issues when len(x)>10 and len(colors)<=10, although we could work out a solution.

Copy link
Owner

Choose a reason for hiding this comment

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

In my original version, I assume that users make sure that len(colors)=len(x) , if they use colors. Therefore, I didn't expect the issue len(x)>10 and len(colors)<=10.

My original idea is that:
(1) if colors is None
(1a) if len(x)<=10, use d3.js category10 colors;
(1b) else, quit. I used to think that it is not a good idea to plot a chord diagram with too many categories, which makes the chords too many to see individual ones. Maybe I am wrong at this point. At lease, I have the keyword colors to rescue this.
(2) if colors is given by users, len(colors)=len(x), and the colors are sometimes meaningful, e.g., colors are chosen to be consistent with other panels. In this case, everything is left to the users.

We can think about the following options:

  1. Maybe we can use your cmap if len(x)>10, and d3.js category10 if len(x)<10?
  2. Maybe we can use d3.js category20 colors if 10<len(x)<=20; and raise an error if len(x)>20?

Let's also assert len(colors)==len(x) if not colors is None.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I think the best idea is prioritize "colors" vs "colormap". I mean, by default (with no colormap or colors specified) use category10 if len(x)<=10, category20 if 10<len(x)<=20 and gist_rainbow (better than rainbow) if len(x)>20. It is indeed difficult to visualize if there are more than 15 categories, but sometimes it is inevitable.

I guess I will remove the colormap option and use colors instead. I mean:

if type(colors) is list:
      # use the list of colors (and `assert len(colors)==len(x)`)
elif type(colors) is str:
      # use the colormap specified
else: 
      #use the default colors specified

It should be easier that way. What do you think about this solution?

Copy link
Owner

Choose a reason for hiding this comment

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

Great idea. I think this solution will also simplify the user interface for the function. Please implement it. Thanks a lot.

if colors is None:
# use d3.js category10 https://github.com/d3/d3-3.x-api-reference/blob/master/Ordinal-Scales.md#category10

import matplotlib.pyplot as plt
Copy link
Owner

Choose a reason for hiding this comment

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

plt has already been imported in line 3.

Copy link
Author

Choose a reason for hiding this comment

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

Sure! I'll change that.

if (len(x) > 10) or (colors is None):
cm = plt.get_cmap(cmap)
colors = cm(np.linspace(0,1,len(x)))[:,:3]
else:
colors = ['#1f77b4', '#ff7f0e', '#2ca02c', '#d62728', '#9467bd',
Copy link
Owner

Choose a reason for hiding this comment

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

The d3.js category10 colors have been used as the default colors for matplotlib v2.x. It has a better visualization than sequential color map to represent category data.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, is there any colormap in matplotlib including the d3.js category10? I couldn't find them..

Copy link
Owner

Choose a reason for hiding this comment

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

There is no color map for d3.js category10 in matplotlib. They are used as the default color cycles for plt.plot(). Please check this: https://matplotlib.org/users/dflt_style_changes.html#colors-color-cycles-and-color-maps

Usually colormap is used for continuous variables, rather than category data. Therefore, my opinion is to use category10 to represent the data if there are less than 10 categories.

Copy link
Author

Choose a reason for hiding this comment

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

Perfect then, we'll stick with category10 if less than 10 categories and no colormap specified.

@pakitochus
Copy link
Author

The requested changes have been added. I slightly changed the code, prioritizing category10 and asserting afterwards if len(x)<=len(colors) to raise an error if there were more categories than colors. Added option for colormap as keyword in the same variable (colors).

@fengwangPhysics
Copy link
Owner

Thanks. The changes in matplotlib-chord.py is good. Sorry for the late reply.

I don't have experience on PR, so I am not sure whether it will interfere your PR if I directly change README.md. So please also update README.md and example.png accordingly.

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.

2 participants