taborlab / FlowCal

Python Flow Cytometry Calibration Library
MIT License
48 stars 23 forks source link

Remove palettable #328

Closed JS3xton closed 4 years ago

JS3xton commented 4 years ago

Remove dependency on palettable and replace it with similar alternatives from matplotlib. Closes https://github.com/taborlab/FlowCal/issues/181.

Comparison of plots produced by >python -m FlowCal.excel_ui -p -i ./examples/experiment.xlsx: Before pull request (9cd83ef7fadf235e548bc6c1bb5ed22d4dde8416) After pull request (e44e9c8ef59b04f5a45646c0e6b897426224d241)

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

castillohair commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

New commit (c02b5d0898adc03e04e24e890c5f873c714a2d9f) 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 commented 4 years ago

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

castillohair commented 4 years ago

Changed standard curve colors for modern equivalents.

Current develop (9cd83ef7fadf235e548bc6c1bb5ed22d4dde8416): std_crv_FL1_B0001

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

After my update (833a099daaa0f2c9325dea524e830a7368398e68): std_crv_FL1_B0001 SMC

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

Will merge now.