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

Remove palettable #328

Merged
merged 5 commits into from
Jun 13, 2020
Merged

Remove palettable #328

merged 5 commits into from
Jun 13, 2020

Conversation

JS3xton
Copy link
Contributor

@JS3xton JS3xton commented May 6, 2020

Remove dependency on palettable and replace it with similar alternatives from matplotlib. Closes #181.

Comparison of plots produced by >python -m FlowCal.excel_ui -p -i ./examples/experiment.xlsx:

Before pull request (9cd83ef) After pull request (e44e9c8)

I.e. blue now looks purple, but otherwise it's similar.

JS3xton added 2 commits May 6, 2020 13:01
In an attempt to preserve previous behavior as much as possible,
use matplotlib's Spectral_r colormap, which resembles the Color
Brewer diverging spectral map we were previously using from
palettable.
@JS3xton JS3xton requested a review from castillohair May 6, 2020 23:12
@castillohair
Copy link
Collaborator

Besides the individual comments, I like the results in most cases. Going all the way to purple allows better differentiation between the first few subpopulations.

The exception is 1D histograms. This might be subjective, but I don't like that the first plot is purple. Would it be better to have 1D histograms follow matplotlib's default color cycle? Meaning first one would be blue, second orange, third green, etc.

@JS3xton
Copy link
Contributor Author

JS3xton commented May 7, 2020

Sorry, I don't completely understand your comment @castillohair.

I understand you like purple for some plots but not others? Specifically, you don't like purple for the 1D histograms (e.g. bottom plot of last image? and maybe 2nd and 3rd plots of second image?). The third image is 1D histograms but they correspond with the subpopulations in the first image and color coordination is helpful. What do you propose there?

Recall these colors all come from a module-level cmap_default variable. If you want to use different color maps for different sets of plots, that would probably have to change. Or are you saying all colormaps should be the matplotlib default?

I personally don't have strong feelings either way regarding the blue -> purple change. I have a weak preference for preserving previous behavior (i.e. using Spectral_r, which resembles the previous colormap, over the matplotlib default). I don't love the purple, but I figured I'd get used to it.

We could also probably make a slightly modified Spectral_r colormap that doesn't go all the way to purple, which would even more closely resemble the old colormap. I prefer the simplicity of embracing the standard Spectral_r map, though, over making a custom solution.

@castillohair
Copy link
Collaborator

I'm talking specifically about 1D histograms that result when calling density_and_hist(), i.e. 2nd and 5th rows in your table. These are currently generated from cmap_default as follows:

colors = [cmap_default(i) for i in np.linspace(0, 1, n_colors)]

But there's no reason these should come out of a colormap because they don't appear in the same axis. So I propose these to be taken from the current color cycle, which would normally result in blue, orange, green, red, etc. It seems that this could be done as follows:

colors = ['C{}'.format(i) for i in range(n_colors)]

See this and this. Although I don't know if these would roll over if you have more plots than the color cycle, which is what happens when you call plt.plot() with too many lines. Also, which is the oldest matplotlib version that supports this.

Everything else looks great to me.

@JS3xton
Copy link
Contributor Author

JS3xton commented May 9, 2020

New commit (c02b5d0) should achieve what you've asked for.

I used

default_property_cycler = plt.rcParams['axes.prop_cycle']()
colors = [next(default_property_cycler)['color'] for i in range(n_colors)]

instead of

colors = ['C{}'.format(i) for i in range(n_colors)]

which should wrap around. E.g.

n_colors = 15
default_property_cycler = plt.rcParams['axes.prop_cycle']()
colors = [next(default_property_cycler)['color'] for i in range(n_colors)]

# colors = ['#1f77b4', '#ff7f0e', '#2ca02c', '#d62728', '#9467bd', '#8c564b', '#e377c2', '#7f7f7f', '#bcbd22', '#17becf', '#1f77b4', '#ff7f0e', '#2ca02c', '#d62728', '#9467bd']

New plots 2 and 5:

@JS3xton
Copy link
Contributor Author

JS3xton commented May 10, 2020

The images in the documentation should also probably be updated once we have the behavior (colormaps) we want.

@castillohair
Copy link
Collaborator

Changed standard curve colors for modern equivalents.

Current develop (9cd83ef):
std_crv_FL1_B0001

After @JS3xton 's last commit (c02b5d0):
std_crv_FL1_B0001 copy

After my update (833a099):
std_crv_FL1_B0001 SMC

Also confirmed that everything works with our oldest matplotlib supported version, 2.0.0.

Will merge now.

@castillohair castillohair merged commit 9071601 into taborlab:develop Jun 13, 2020
@JS3xton JS3xton deleted the remove-palettable branch June 15, 2020 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants