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

Added the from_levels_and_colors function. #2050

Merged
merged 1 commit into from May 28, 2013

Conversation

pelson
Copy link
Member

@pelson pelson commented May 22, 2013

This PR adds a function which simplifies the construction of a cmap and norm for discrete levels with specific colours.
The interface is intentionally similar to the contourf levels and colors arguments - though a little stricter in the fact that the correct number of colors for levels is required.

This has come up a couple of times with my colleagues and I always end up having to write a chunk of test code to make sure I've done it right. This function generally makes this easier - and will hopefully make doing quantized pcolormesh's less error prone.

@@ -189,7 +189,7 @@ def deprecate(func, message=message, name=name, alternative=alternative,
name = func.__name__

message = _generate_deprecation_message(
since, message, name, alternative, pending, 'function')
since, message, name, alternative, pending, obj_type)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching that.

@pelson
Copy link
Member Author

pelson commented May 22, 2013

There is a python3 hashable problem which I need to deal with, but other than that, I'm pretty keen to get this in v1.3.x. For what it's worth, I've put together a gist which demonstrates the benefit: http://nbviewer.ipython.org/5628989

@@ -607,6 +612,11 @@ def __call__(self, X, alpha=None, bytes=False):
rgba = tuple(rgba[0, :])
return rgba

@property
Copy link
Member

Choose a reason for hiding this comment

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

I like this use of properties; is there any reason not to extend it so that it can be used to set as well as get the value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure could do, but that would make two ways of setting the colours. There's no reason that we couldn't go down the deprecation path for set_under, set_over and set_bad (or just have two ways of setting them I suppose).

Copy link
Member

Choose a reason for hiding this comment

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

It seems overly inconsistent to be able to read but not write bad_color etc, since writing is mostly what one wants to do from user code. For the purpose of your function, you don't need an external API--you could just use the private variables.

The rationale for keeping set_bad etc. would be that they include the alpha kwarg, although that is never really needed; one could always use, for example, bad_color = mpl.colors.colorConverter.to_rgba("r", alpha=0.5).

The use of setters and getters is a legacy. There are places where they serve a major function in the auto documentation system, but here in the colors module the are just fossils from early days. (I put them in.) I don't see any urgency to deprecate them, but I think that using them to flesh out the bad_color etc. properties would make sense, if you really want to proceed with bad_color--which I think makes for a much nicer API.

This brings up another point: to_rgba and friends are so deeply hidden that even I can never remember how to access them, but have to hunt around to find them. (A camelCase name? How did we end up with that?) Wouldn't it be nice, from the user standpoint, to have basic functionality like this in some more obvious place--either directly in colors as a function, or maybe even the base matplotlib namespace? Right now, that namespace (from import matplotlib) is full of completely useless junk--throwaways from the startup process. Ugly!

Copy link
Member Author

Choose a reason for hiding this comment

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

I like your outlook @efiring :-)
If you and @mdboom are willing to review and merge it, I'm willing to submit the PRs 😄

In the interests of keeping this PR as short as it can be, I'll add the property setters and hold back from deprecating the set_* methods.

@pelson
Copy link
Member Author

pelson commented May 24, 2013

@efiring - I've added a commit which does some of what you discussed (and more). If it is too extreme, then I'll take it back and be a little more conservative 😉.

I'm also motivated to simplify the color conversion process in matplotlib and to provide a means to create hsv, hsl, hsva and hsvla interpolated color schemes easily. I'm tempted to hold fire on this and do it in v1.4 though (along with deprecating the use of Colormap.set_bad etc.,).

What do you think?

@mdboom
Copy link
Member

mdboom commented May 24, 2013

I think we should hold for 1.4 any further features at this point. (The feature freeze began almost two weeks ago now, and I've let a few things slide through, but we're mainly in the mode of polishing and bugfixing the features we already have at this point.)

@mdboom
Copy link
Member

mdboom commented May 24, 2013

I'll leave it to @efiring to give this one more look over and merge.

@efiring
Copy link
Member

efiring commented May 24, 2013

@pelson, @mdboom, there are lots of good ideas in here, but also some radical changes that I think deserve discussion, so I am not willing to merge this as-is. Most of it may be more appropriate for 1.4 than for 1.3 at this late stage.

The biggest strategy change here is including peek() as a method of the Colormap. Having a peek method or function is a great idea, but making it a Colormap method, and yet including all the high level pyplot functionality while not even returning the figure object, seems to me like a questionable design. Everywhere else, we have tried to keep some high-level to low-level hierarchy. To be consistent with that logic, peek should be a pyplot function, perhaps view_colormap(cbar). (Longer term, it would be nice to have a colormap-editing widget instantiated by a pyplot function.) Having it as a pyplot function would also make it more discoverable.

The decorator gymnastics in the present PR are also getting a bit hard to follow, and I don't think they actually save any LOC or clarify anything, compared to simply repeating the phrase,

        if self._isinit:
            self._set_extremes()

three times, which amounts to 6 LOC, versus many more for the definition of the decorator plus its three invocations. With all the switching around of self and func, that decorator begins to resemble a textbook example of obfuscated code.

@pelson
Copy link
Member Author

pelson commented May 24, 2013

I don't disagree with the analysis @efiring. Some of the things in here are a little contravertial and have no place in v1.3, but I do feel quite passionately about the underlying purpose for this PR making it in (the function from_levels_and_colors which avoids users, i.e. my colleagues, accidentally mis-representing their data by misconstructing Norms and Cmaps for quantised levels.). The first commit on this PR does that, without introducing any controversial changes - perhaps that is the way to go here.

@pelson
Copy link
Member Author

pelson commented May 24, 2013

Ok. This has been reverted to the first commit (my original changes are in a branch https://github.com/pelson/matplotlib/tree/colormap_api_for_v1.4 if anybody is interested).

@efiring
Copy link
Member

efiring commented May 24, 2013

@pelson, for the purpose of inclusion in 1.3, I recommend that you remove the new properties. At this stage, they would represent a commitment to a new API, which might be a good one (basically, I like it), but which is incomplete, and which contributes absolutely nothing to the purpose of the PR--no functionality, no reduction in LOC, no improvement in readability or performance.

You might even go so far as to put an "Experimental" tag in the docstring of your new function, to leave a little wiggle-room. I think this would be wise, given the very short review period that has been available.

@pelson
Copy link
Member Author

pelson commented May 28, 2013

I've now reverted all of the API changes on Colormap.

mdboom added a commit that referenced this pull request May 28, 2013
Added the from_levels_and_colors function.
@mdboom mdboom merged commit 48e1439 into matplotlib:master May 28, 2013
mdboom added a commit that referenced this pull request May 28, 2013
Added the from_levels_and_colors function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants