[PATCH] dysfunction in mpl.cm.get_cmap() (Reporter: LittleBigBrain)

Development matplotlib-devel
1

The changes look ok to me so far. It looks to be mostly a re-organization of existing logic and some consolidation of code. My only concerns are the creation of two new functions. Besides the obvious issues with potential namespace collisions in other parts of the code that might do an ‘from cm import *’, my main issue is that these functions are probably really only meant for internal use and should probably start with an underscore. We can always un-underscore them in a later release…

I will do a bit more testing to see if I can break it, but barring that, I will commit a slightly modified version of the patch later today.

Ben Root

P.S. - As for going off-list in my last email, that was an ‘oops’.

···

On Thu, Jan 6, 2011 at 10:20 AM, Friedrich Romstedt <friedrichromstedt@…149…> wrote:

Hi Ben,

nice to get a response. It’s not an urgent patch, but should go in

somwhen (imo). If you could take it and verify that it breaks nothing

I would owe you a donut :wink:

Why OL?

Here’s the reference:

http://thread.gmane.org/gmane.comp.python.matplotlib.general/25103

Notice that in the email from 18 Oct 14:55, I gave the wrong branch on

GitHub, it’s https://github.com/friedrichromstedt/matplotlib/tree/friedrichromstedt-get_cmap

apparently (not “trunk” branch).

Shall I add it to the bug tracker so that you can assign it maybe to yourself?

I long for a matplotlib GitHub repo … You might use the GitHub

system to review the patch (it’s more readable than a diff file I

guess). Here’s the link:

https://github.com/friedrichromstedt/matplotlib/commit/1ac794b362d143ac6a634b70993d950d8fe4adeb.

Feel free to go on-list, I did not snip away at your response so that

the on-list history will be complete.

Friedrich

2011/1/6 Benjamin Root <ben.root@…55…553…>:

On Mon, Jan 3, 2011 at 2:36 PM, Friedrich Romstedt > > > <friedrichromstedt@…55…149…> wrote:

2010/10/19 Friedrich Romstedt <friedrichromstedt@…149…>:

The test suite passes except for some failures on my 10.6 Mac OS X

with yesterday checked-out astraw github repo. I’ll send them in

another thread. Let’s keep tidy :slight_smile: There’s more than the symlog

error mentioned by LittleBigBrain on my sys.

The failures are identical with and without the patch.

Opinions?

There has been no response, so I’m resending together with the patch

file. The patch is created by $ git format-patch -1 on my old git

branch from that time, and can be reapplied by:

$ patch -p1 <0001-Fixing-bug-in-mpl.cm.get_cmap.patch

I cannot test it against recent svn because my gcc on Mac OS X went

havoc after restoring the system from Time Machine (the HDD crashed).

Does anybody know what to do when it cannot find even stdarg.h ???

That time the failures of the test suite did not increase by

introducing the change. And it fixes LittleBigBrain’s bug.

Annotations from the git commit are in the patch file. I think it

needs some thinking-through, although I did it carefully, to approve

it.

Btw what happened to the astraw github mirror? The latest commit on

branch “trunk” is from November.

Friedrich

Friedrich,

I don’t know why you haven’t gotten a response on this. I have not looked

into it at all though. Is it a patch that needs to go into 1.0.1? If so, I

can make sure it gets there.

Ben Root

2

2011/1/8 Benjamin Root <ben.root@...553...>:

The changes look ok to me so far. It looks to be mostly a re-organization
of existing logic and some consolidation of code. My only concerns are the
creation of two new functions. Besides the obvious issues with potential
namespace collisions in other parts of the code that might do an 'from cm
import *', my main issue is that these functions are probably really only
meant for internal use and should probably start with an underscore. We can
always un-underscore them in a later release...

I agree on that the functions are kind of internal. Atm they do not
reach their aim of generating cmaps from arbitrary specifications
(generate_cmap can only handle known cmaps by name). It seems to me I
was too fast and did not think it thoroughly through :-/.

I'm not really satisfied with the current state of the patch with the
informations you gave at hand. I wrote this patch as it is because I
assumed functions like ``revcmap`` are supposed to stay
backward-compatible.

If backward compatibility isn't an issue for this functions, so why
not going ahead and rewriting the functions so that they deliver some
useful functionality instead of only helping functionality. And if
there are non-public stuff functions remaining, we could place an
__all__ at the top of the file, to prevent them being imported by
``from cm import *``.

I will do a bit more testing to see if I can break it, but barring that, I
will commit a slightly modified version of the patch later today.

:-/ You are too fast for me.

In fact I think this functions belong to
colors.LinearSegmentedColormap. Reversing should be a method of that
one, and they should accept directly all kind of specifications at
initialisation time.

So what parts should stay backwards compatible and which are we able to change?

* Do we have to keep LinearSegmentedColormap.from_list() or should it
be simply an alias for its __init__()?
* What is the callable-functionality in cm.revcmap for? It can
handle callable value items, but where's this used in mpl?
* And, as said, do we have to keep this list-and-dicitionary mangling
functions outside of LinearSegmentedColormap? I.e., can we replace it
by reversing fully-fledged Colormaps, instead of reversing their
tuple-or-dict specs?

Friedrich

3

2011/1/8 Benjamin Root <ben.root@…55…553…>:

The changes look ok to me so far. It looks to be mostly a re-organization

of existing logic and some consolidation of code. My only concerns are the

creation of two new functions. Besides the obvious issues with potential

namespace collisions in other parts of the code that might do an 'from cm

import *', my main issue is that these functions are probably really only

meant for internal use and should probably start with an underscore. We can

always un-underscore them in a later release…

I agree on that the functions are kind of internal. Atm they do not

reach their aim of generating cmaps from arbitrary specifications

(generate_cmap can only handle known cmaps by name). It seems to me I

was too fast and did not think it thoroughly through :-/.

I’m not really satisfied with the current state of the patch with the

informations you gave at hand. I wrote this patch as it is because I

assumed functions like revcmap are supposed to stay

backward-compatible.

Any existing function should remain backwards compatible, and so I have no intention of putting an underscore on that one. Only on the new ones.

If backward compatibility isn’t an issue for this functions, so why

not going ahead and rewriting the functions so that they deliver some

useful functionality instead of only helping functionality. And if

there are non-public stuff functions remaining, we could place an

all at the top of the file, to prevent them being imported by

from cm import *.

That might be a style issue that would be up to other maintainers. Personally, at the very least, any function that is meant to be internal should start with an underscore just to act as a flag to others. But then using the all approach gives the added benefit of making the documentation cleaner.

I will do a bit more testing to see if I can break it, but barring that, I

will commit a slightly modified version of the patch later today.

:-/ You are too fast for me.

Actually, I haven’t committed anything yet. I have a bit of a backlog here…

In fact I think this functions belong to

colors.LinearSegmentedColormap. Reversing should be a method of that

one, and they should accept directly all kind of specifications at

initialisation time.

Actually, I think it belongs down in Colormap. I don’t see why this should be limited to just LinearSegmentedColormap.

So what parts should stay backwards compatible and which are we able to change?

  • Do we have to keep LinearSegmentedColormap.from_list() or should it

be simply an alias for its init()?

  • What is the callable-functionality in cm.revcmap for? It can

handle callable value items, but where’s this used in mpl?

  • And, as said, do we have to keep this list-and-dicitionary mangling

functions outside of LinearSegmentedColormap? I.e., can we replace it

by reversing fully-fledged Colormaps, instead of reversing their

tuple-or-dict specs?

My feeling is that for the purposes of 1.0.1, what you put together is good (although with those new functions underscored). I will then merge that over to the development branch. In the development branch, we can then make any additional design changes. As far as the 1.0.1 release goes, there really shouldn’t be any new functionalities, only bug fixes for existing code.

Ben Root

···

On Mon, Jan 10, 2011 at 7:46 AM, Friedrich Romstedt <friedrichromstedt@…149…> wrote:

4

[...]

My feeling is that for the purposes of 1.0.1, what you put together is
good (although with those new functions underscored). I will then merge
that over to the development branch. In the development branch, we can
then make any additional design changes. As far as the 1.0.1 release
goes, there really shouldn't be any new functionalities, only bug fixes
for existing code.

Do you mean 1.0.2? John tagged 1.0.1 4 days ago, it is listed as available for download, and I see that a couple of Mac dmg versions were added within the last hour.

Eric

···

On 01/10/2011 08:02 AM, Benjamin Root wrote:

Ben Root

5

Finally got around to committing this. I committed it to the maintenance branch in r8933 and merged into the development branch in r8934.

Ben Root

···

On Mon, Jan 10, 2011 at 7:46 AM, Friedrich Romstedt <friedrichromstedt@…149…> wrote:

2011/1/8 Benjamin Root <ben.root@…55…553…>:

The changes look ok to me so far. It looks to be mostly a re-organization

of existing logic and some consolidation of code. My only concerns are the

creation of two new functions. Besides the obvious issues with potential

namespace collisions in other parts of the code that might do an 'from cm

import *', my main issue is that these functions are probably really only

meant for internal use and should probably start with an underscore. We can

always un-underscore them in a later release…

I agree on that the functions are kind of internal. Atm they do not

reach their aim of generating cmaps from arbitrary specifications

(generate_cmap can only handle known cmaps by name). It seems to me I

was too fast and did not think it thoroughly through :-/.

I’m not really satisfied with the current state of the patch with the

informations you gave at hand. I wrote this patch as it is because I

assumed functions like revcmap are supposed to stay

backward-compatible.

If backward compatibility isn’t an issue for this functions, so why

not going ahead and rewriting the functions so that they deliver some

useful functionality instead of only helping functionality. And if

there are non-public stuff functions remaining, we could place an

all at the top of the file, to prevent them being imported by

from cm import *.

I will do a bit more testing to see if I can break it, but barring that, I

will commit a slightly modified version of the patch later today.

:-/ You are too fast for me.

In fact I think this functions belong to

colors.LinearSegmentedColormap. Reversing should be a method of that

one, and they should accept directly all kind of specifications at

initialisation time.

So what parts should stay backwards compatible and which are we able to change?

  • Do we have to keep LinearSegmentedColormap.from_list() or should it

be simply an alias for its init()?

  • What is the callable-functionality in cm.revcmap for? It can

handle callable value items, but where’s this used in mpl?

  • And, as said, do we have to keep this list-and-dicitionary mangling

functions outside of LinearSegmentedColormap? I.e., can we replace it

by reversing fully-fledged Colormaps, instead of reversing their

tuple-or-dict specs?

Friedrich

6

2011/1/22 Benjamin Root <ben.root@...553...>:

Finally got around to committing this. I committed it to the maintenance
branch in r8933 and merged into the development branch in r8934.

Okay, I agree on this, but I will work on it again once the git
transition is complete, or similar.

Thanks,
Friedrich