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

Optional cmaps #567

Merged
merged 1 commit into from Mar 25, 2015

Conversation

Projects
None yet
4 participants
@jalabort
Member

jalabort commented Mar 23, 2015

In Menpo, 1 channel images are always rendered using a grey-scale colormap. This PR allows the user to optionally pass the colormap under which a Menpo image should be rendered as a kwarg in its view method.

Note that the previous kwarg can also be passed in case of multi channel images and that previously default behaviours are preserved.

@jabooth jabooth added the in progress label Mar 23, 2015

alpha=alpha)
if len(self.image.shape) == 2:
if not cmap:
# Single channels are viewed in Gray by default

This comment has been minimized.

@nontas

nontas Mar 23, 2015

Member

This should be if cmap is None right?

This comment has been minimized.

@jalabort

jalabort Mar 24, 2015

Member

I thought they were equivalent but if cmap is None is more explicit and probably better. I will change this.

@nontas

This comment has been minimized.

Member

nontas commented Mar 23, 2015

I think that this is a good addition! However, I believe that the cmap argument shouldn't expect a matplotlib.cm object as it is done now. It should get an str and the colourmap itself should be generated within the rendering method. Specifically, matplotlib has a special method to get a coloumap from its name as:

from matplotlib import cm
cmap = cm.get_cmap(cmap_name)

where possible cmap_name values are:
Spectral, summer, coolwarm, Wistia_r, pink_r, Set1, Set2, Set3, brg_r, Dark2, prism, PuOr_r, afmhot_r, terrain_r, PuBuGn_r, RdPu, gist_ncar_r, gist_yarg_r, Dark2_r, YlGnBu, RdYlBu, hot_r, gist_rainbow_r, gist_stern, PuBu_r, cool_r, cool, gray, copper_r, Greens_r, GnBu, gist_ncar, spring_r, gist_rainbow, gist_heat_r, Wistia, OrRd_r, CMRmap, bone, gist_stern_r, RdYlGn, Pastel2_r, spring, terrain, YlOrRd_r, Set2_r, winter_r, PuBu, RdGy_r, spectral, rainbow, flag_r, jet_r, RdPu_r, gist_yarg, BuGn, Paired_r, hsv_r, bwr, cubehelix, Greens, PRGn, gist_heat, spectral_r, Paired, hsv, Oranges_r, prism_r, Pastel2, Pastel1_r, Pastel1, gray_r, jet, Spectral_r, gnuplot2_r, gist_earth, YlGnBu_r, copper, gist_earth_r, Set3_r, OrRd, gnuplot_r, ocean_r, brg, gnuplot2, PuRd_r, bone_r, BuPu, Oranges, RdYlGn_r, PiYG, CMRmap_r, YlGn, binary_r, gist_gray_r, Accent, BuPu_r, gist_gray, flag, bwr_r, RdBu_r, BrBG, Reds, Set1_r, summer_r, GnBu_r, BrBG_r, Reds_r, RdGy, PuRd, Accent_r, Blues, autumn_r, autumn, cubehelix_r, nipy_spectral_r, ocean, PRGn_r, Greys_r, pink, binary, winter, gnuplot, RdYlBu_r, hot, YlOrBr, coolwarm_r, rainbow_r, Purples_r, PiYG_r, YlGn_r, Blues_r, YlOrBr_r, seismic, Purples, seismic_r, RdBu, Greys, BuGn_r, YlOrRd, PuOr, PuBuGn, nipy_spectral, afmhot.

What do you think?

Btw, after we decide on this, I will update the widgets accordingly.

@jalabort

This comment has been minimized.

Member

jalabort commented Mar 24, 2015

I also agree on this. I will change it to receive a str and call cm.get_cmap inside.

Also let's fix this and pull it in and deal with the widgets on a separate PR.

@@ -530,7 +531,9 @@ def _view_2d(self, figure_id=None, new_figure=False, channels=None,
{none, nearest, bilinear, bicubic, spline16, spline36,
hanning, hamming, hermite, kaiser, quadric, catrom, gaussian,
bessel, mitchell, sinc, lanczos}
cmap_name: `Colormap name`, optional,

This comment has been minimized.

@jabooth

jabooth Mar 25, 2015

Member

shouldn't this be str, optional?

This comment has been minimized.

@jalabort

jalabort Mar 25, 2015

Member

So true!

@jabooth

This comment has been minimized.

Member

jabooth commented Mar 25, 2015

Great addition, just that tiny doc clarification and I'm happy. +1

@@ -651,6 +654,9 @@ def _view_landmarks_2d(self, channels=None, group=None,
hamming, hermite, kaiser, quadric, catrom, gaussian, bessel,
mitchell, sinc, lanczos}
cmap_name: `Colormap name`, optional,

This comment has been minimized.

@nontas

nontas Mar 25, 2015

Member

Same here as @jabooth comment. It should be str

@nontas

This comment has been minimized.

Member

nontas commented Mar 25, 2015

Looks good to me! +1

patricksnape added a commit that referenced this pull request Mar 25, 2015

@patricksnape patricksnape merged commit 542dca0 into menpo:master Mar 25, 2015

2 of 3 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jalabort jalabort deleted the jalabort:optional_cmaps branch Jul 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment