clabel improvements

Hi,

Well, I now know more than I ever wanted to about clabel. I decided to
improve a bit on the inlining and ended up rewriting it. For automatic
label placement, I basically use the existing algorithm for determining
label location, but have replaced existing code for determining the
angle of rotation and the amount of the contour to take off for inlining
with new code. This new code is based on using pixel-space distances
along the contour. This allows one to (1) get nice balanced inlining
with the same amount of space on either side of the label, and (2) to
vary the amount of space you want around the label. It also should deal
better with labels located near contour edges and labels covering the
entire contour.

Along the way, I renamed all ContourLabeler specific attributes to
something like .labelAttribute. This makes the namespace cleaner in my
mind, but might break existing user code that does things directly with
ContourLabeler attributes.

I also added a few new functions to cbook. One does simple linear
interpolation (in addition to an existing cbook function that is similar
but a bit different). Others do things with vector lengths. I also
added a function called isvector that is identical to a Matlab function
of the same name. If desired, I can move this to mlab.py, but for the
moment it is in cbook.py because most of the is? functions are there.

On an aside, I noted that mlab.norm uses cut-and-paste documentation
from Matlab. Is this wise?

I have tested all the changes against the existing pylab_examples and
things work fine. Nonetheless, since lots of things have been changed,
I haven't committed them for fear of interfering with the release.
Instead, I am attaching the patch set. If I get the green light, I will
commit these changes.

Related: while I am digging around in there, now is probably the moment
for me to integrate Paul Kienzle's comments on start/stop_event_loop in
FigureCanvasBase, etc. I am not sure there is a consensus on this. I
am currently thinking that the best way to integrate this, while
minimizing repeated code, is a mixin plus a couple of new classes,
FigureCanvasBaseGUI and FigureCanvasGUIAgg. These new classes would
inherit the mixin and the base classes without "GUI". Interactive
backends would then inherit these. Non-interactive backends would
inherit versions that throw errors from FigureBaseCanvas. Complex, but
this assures clean inheritance. Thoughts welcome.

Cheers,
David

clabel_rewrite.patch (27.5 KB)

···

--
**********************************
David M. Kaplan
Charge de Recherche 1
Institut de Recherche pour le Developpement
Centre de Recherche Halieutique Mediterraneenne et Tropicale
av. Jean Monnet
B.P. 171
34203 Sete cedex
France

Phone: +33 (0)4 99 57 32 27
Fax: +33 (0)4 99 57 32 95
http://www.ur097.ird.fr/team/dkaplan/index.html
**********************************

Hi,

Well, I now know more than I ever wanted to about clabel. I decided to
improve a bit on the inlining and ended up rewriting it. For automatic
label placement, I basically use the existing algorithm for determining
label location, but have replaced existing code for determining the
angle of rotation and the amount of the contour to take off for inlining
with new code. This new code is based on using pixel-space distances
along the contour. This allows one to (1) get nice balanced inlining
with the same amount of space on either side of the label, and (2) to
vary the amount of space you want around the label. It also should deal
better with labels located near contour edges and labels covering the
entire contour.

Along the way, I renamed all ContourLabeler specific attributes to
something like .labelAttribute. This makes the namespace cleaner in my
mind, but might break existing user code that does things directly with
ContourLabeler attributes.

Eric, do you have any sense of whether people would use this directly?
Since this is a point release, I want to minimize API breakage, so at
least keep the old attrs around for this cycle. Eg, in Axes I
recently renamed axesPatch to simply patch as follows:

        # the patch draws the background of the axes. we want this to
        # be below the other artists; the axesPatch name is deprecated
        self.patch = self.axesPatch = self._gen_axes_patch()

we don't have an easy way to catch deprecated usage w/o some property
or getattr magic. If you want to add getter properties for the
deprecated attrs which warn and point to the new, that would be ideal.

I also added a few new functions to cbook. One does simple linear
interpolation (in addition to an existing cbook function that is similar
but a bit different). Others do things with vector lengths. I also
added a function called isvector that is identical to a Matlab function
of the same name. If desired, I can move this to mlab.py, but for the
moment it is in cbook.py because most of the is? functions are there.

That's fine. Is this different from "iterable"

On an aside, I noted that mlab.norm uses cut-and-paste documentation
from Matlab. Is this wise?

No, please rewrite the docstring. Some of mlab was borrowed form
other people's codes, and I was unaware of this.

I have tested all the changes against the existing pylab_examples and
things work fine. Nonetheless, since lots of things have been changed,
I haven't committed them for fear of interfering with the release.
Instead, I am attaching the patch set. If I get the green light, I will
commit these changes.

I'll leve the final call on this to Eric, but after this I suggest we
implement a feature freeze until after we get the current code tested
and out. Friday is a reasonable target if others agree and we have a
chance to test this for a couple of days.

Related: while I am digging around in there, now is probably the moment
for me to integrate Paul Kienzle's comments on start/stop_event_loop in
FigureCanvasBase, etc. I am not sure there is a consensus on this. I
am currently thinking that the best way to integrate this, while
minimizing repeated code, is a mixin plus a couple of new classes,
FigureCanvasBaseGUI and FigureCanvasGUIAgg. These new classes would
inherit the mixin and the base classes without "GUI". Interactive
backends would then inherit these. Non-interactive backends would
inherit versions that throw errors from FigureBaseCanvas. Complex, but
this assures clean inheritance. Thoughts welcome.

See my response in the original thread. I'm not sure we ever reached
a consensus on this one, and I'm still uncomfortable with a more
complex hierarchy. I'm willing to be convince if you and Paul and
Gael disagree, but I have yet to see why a flat implementation will
not work here.

JDH

···

On Wed, Jul 23, 2008 at 6:21 AM, David Kaplan <David.Kaplan@...622...> wrote:

John Hunter wrote:

Hi,

Well, I now know more than I ever wanted to about clabel. I decided to
improve a bit on the inlining and ended up rewriting it. For automatic
label placement, I basically use the existing algorithm for determining
label location, but have replaced existing code for determining the
angle of rotation and the amount of the contour to take off for inlining
with new code. This new code is based on using pixel-space distances
along the contour. This allows one to (1) get nice balanced inlining
with the same amount of space on either side of the label, and (2) to
vary the amount of space you want around the label. It also should deal
better with labels located near contour edges and labels covering the
entire contour.

It all sounds like much-needed improvement.

Along the way, I renamed all ContourLabeler specific attributes to
something like .labelAttribute. This makes the namespace cleaner in my
mind, but might break existing user code that does things directly with
ContourLabeler attributes.

Eric, do you have any sense of whether people would use this directly?

I think the probability that anyone is doing this is low, but it would be nice to ask on the users mailing list.

Since this is a point release, I want to minimize API breakage, so at
least keep the old attrs around for this cycle. Eg, in Axes I
recently renamed axesPatch to simply patch as follows:

        # the patch draws the background of the axes. we want this to
        # be below the other artists; the axesPatch name is deprecated
        self.patch = self.axesPatch = self._gen_axes_patch()

we don't have an easy way to catch deprecated usage w/o some property
or getattr magic. If you want to add getter properties for the
deprecated attrs which warn and point to the new, that would be ideal.

In general, yes, but in this case I think it would be better to go with your simpler method above, of making duplicate names. It reduces the code clutter and the chance of introducing last-minute errors. The mailing list and API_CHANGES can be used to notify users of the upcoming deprecation. If there is an outcry, indicating wide use of the attributes, then properties can be introduced later to smooth the deprecation process. But I predict there will be hardly a peep.

I also added a few new functions to cbook. One does simple linear
interpolation (in addition to an existing cbook function that is similar
but a bit different). Others do things with vector lengths. I also

Maybe explain in the docstring or a comment why this version is needed?

added a function called isvector that is identical to a Matlab function

Maybe use try/except to return False if the test fails? If the input is a string, or None, for example? Whether to do this depends on what the use case is.

of the same name. If desired, I can move this to mlab.py, but for the
moment it is in cbook.py because most of the is? functions are there.

That's fine. Is this different from "iterable"

On an aside, I noted that mlab.norm uses cut-and-paste documentation
from Matlab. Is this wise?

No, please rewrite the docstring. Some of mlab was borrowed form
other people's codes, and I was unaware of this.

norm is one of the things we should not have at all; ours should be deprecated in favor of numpy.linalg.norm. It would be good to have a linear algebra guru look at this to see whether a straight substitution with deprecation warning would work, or whether there are significant differences. If something close to a substitution will work, then the docstring in mlab can be replaced with a reference to the numpy function. And then in some future release, it will all be deleted.

I have tested all the changes against the existing pylab_examples and
things work fine. Nonetheless, since lots of things have been changed,
I haven't committed them for fear of interfering with the release.
Instead, I am attaching the patch set. If I get the green light, I will
commit these changes.

Given the stated "release early, release often" strategy, go ahead and commit.

Eric

···

On Wed, Jul 23, 2008 at 6:21 AM, David Kaplan <David.Kaplan@...622...> wrote:

I'll leve the final call on this to Eric, but after this I suggest we
implement a feature freeze until after we get the current code tested
and out. Friday is a reasonable target if others agree and we have a
chance to test this for a couple of days.

Related: while I am digging around in there, now is probably the moment
for me to integrate Paul Kienzle's comments on start/stop_event_loop in
FigureCanvasBase, etc. I am not sure there is a consensus on this. I
am currently thinking that the best way to integrate this, while
minimizing repeated code, is a mixin plus a couple of new classes,
FigureCanvasBaseGUI and FigureCanvasGUIAgg. These new classes would
inherit the mixin and the base classes without "GUI". Interactive
backends would then inherit these. Non-interactive backends would
inherit versions that throw errors from FigureBaseCanvas. Complex, but
this assures clean inheritance. Thoughts welcome.

See my response in the original thread. I'm not sure we ever reached
a consensus on this one, and I'm still uncomfortable with a more
complex hierarchy. I'm willing to be convince if you and Paul and
Gael disagree, but I have yet to see why a flat implementation will
not work here.

JDH

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/matplotlib-devel

John Hunter wrote:

I have tested all the changes against the existing pylab_examples and
things work fine. Nonetheless, since lots of things have been changed,
I haven't committed them for fear of interfering with the release.
Instead, I am attaching the patch set. If I get the green light, I will
commit these changes.

I'll leve the final call on this to Eric, but after this I suggest we
implement a feature freeze until after we get the current code tested
and out. Friday is a reasonable target if others agree and we have a
chance to test this for a couple of days.

+1

Are we going to try to get anything out after that for scipy, or is this it? I realize with Friday we're targeting Debian.

Ryan

···

--
Ryan May
Graduate Research Assistant
School of Meteorology
University of Oklahoma

I actually lean on the simple side for now. If we find as we improve the
event-loop related stuff that we need this, we can easily add an
abstraction, but right now I don't see a big benefit. Anyhow, whoever
does the work gets to decide, as usual.

Ga�l

···

On Wed, Jul 23, 2008 at 09:14:46AM -0500, John Hunter wrote:

> Related: while I am digging around in there, now is probably the moment
> for me to integrate Paul Kienzle's comments on start/stop_event_loop in
> FigureCanvasBase, etc. I am not sure there is a consensus on this. I
> am currently thinking that the best way to integrate this, while
> minimizing repeated code, is a mixin plus a couple of new classes,
> FigureCanvasBaseGUI and FigureCanvasGUIAgg. These new classes would
> inherit the mixin and the base classes without "GUI". Interactive
> backends would then inherit these. Non-interactive backends would
> inherit versions that throw errors from FigureBaseCanvas. Complex, but
> this assures clean inheritance. Thoughts welcome.

See my response in the original thread. I'm not sure we ever reached
a consensus on this one, and I'm still uncomfortable with a more
complex hierarchy. I'm willing to be convince if you and Paul and
Gael disagree, but I have yet to see why a flat implementation will
not work here.

Hi,

I committed the changes to clabel (r5830). For the attribute renaming,
I think we can safely rename most of them. In my opinion, the only ones
that users might use are .cl, .cl_xy and .cl_cvalues. The clabel
function creates these just before finishing from their more
appropriately named versions. This should provide enough backward
compatibility for a release or two.

And yes, isvector is different from iterable. For example:

In [81]: cbook.isvector( array([3,4,5,6]).reshape(1,1,1,4) )
Out[81]: True

In [82]: cbook.isvector( array([3,4,5,6]).reshape(1,1,2,2) )
Out[82]: False

This function is probably more useful in matlab since it has no shape
way to distinguish vectors from 2D matrices, but still it doesn't hurt
to have it around.

Cheers,
David

···

On Wed, 2008-07-23 at 08:08 -1000, Eric Firing wrote:

John Hunter wrote:
> On Wed, Jul 23, 2008 at 6:21 AM, David Kaplan <David.Kaplan@...622...> wrote:
>> Hi,
>>
>> Well, I now know more than I ever wanted to about clabel. I decided to
>> improve a bit on the inlining and ended up rewriting it. For automatic
>> label placement, I basically use the existing algorithm for determining
>> label location, but have replaced existing code for determining the
>> angle of rotation and the amount of the contour to take off for inlining
>> with new code. This new code is based on using pixel-space distances
>> along the contour. This allows one to (1) get nice balanced inlining
>> with the same amount of space on either side of the label, and (2) to
>> vary the amount of space you want around the label. It also should deal
>> better with labels located near contour edges and labels covering the
>> entire contour.

It all sounds like much-needed improvement.

>>
>> Along the way, I renamed all ContourLabeler specific attributes to
>> something like .labelAttribute. This makes the namespace cleaner in my
>> mind, but might break existing user code that does things directly with
>> ContourLabeler attributes.
>
> Eric, do you have any sense of whether people would use this directly?

I think the probability that anyone is doing this is low, but it would
be nice to ask on the users mailing list.

> Since this is a point release, I want to minimize API breakage, so at
> least keep the old attrs around for this cycle. Eg, in Axes I
> recently renamed axesPatch to simply patch as follows:
>
> # the patch draws the background of the axes. we want this to
> # be below the other artists; the axesPatch name is deprecated
> self.patch = self.axesPatch = self._gen_axes_patch()
>
> we don't have an easy way to catch deprecated usage w/o some property
> or getattr magic. If you want to add getter properties for the
> deprecated attrs which warn and point to the new, that would be ideal.

In general, yes, but in this case I think it would be better to go with
your simpler method above, of making duplicate names. It reduces the
code clutter and the chance of introducing last-minute errors. The
mailing list and API_CHANGES can be used to notify users of the upcoming
deprecation. If there is an outcry, indicating wide use of the
attributes, then properties can be introduced later to smooth the
deprecation process. But I predict there will be hardly a peep.

>
>
>> I also added a few new functions to cbook. One does simple linear
>> interpolation (in addition to an existing cbook function that is similar
>> but a bit different). Others do things with vector lengths. I also
Maybe explain in the docstring or a comment why this version is needed?
>> added a function called isvector that is identical to a Matlab function
Maybe use try/except to return False if the test fails? If the input is
a string, or None, for example? Whether to do this depends on what the
use case is.
>> of the same name. If desired, I can move this to mlab.py, but for the
>> moment it is in cbook.py because most of the is? functions are there.
>
> That's fine. Is this different from "iterable"
>
>> On an aside, I noted that mlab.norm uses cut-and-paste documentation
>> from Matlab. Is this wise?
>
> No, please rewrite the docstring. Some of mlab was borrowed form
> other people's codes, and I was unaware of this.

norm is one of the things we should not have at all; ours should be
deprecated in favor of numpy.linalg.norm. It would be good to have a
linear algebra guru look at this to see whether a straight substitution
with deprecation warning would work, or whether there are significant
differences. If something close to a substitution will work, then the
docstring in mlab can be replaced with a reference to the numpy
function. And then in some future release, it will all be deleted.

>
>> I have tested all the changes against the existing pylab_examples and
>> things work fine. Nonetheless, since lots of things have been changed,
>> I haven't committed them for fear of interfering with the release.
>> Instead, I am attaching the patch set. If I get the green light, I will
>> commit these changes.
>
Given the stated "release early, release often" strategy, go ahead and
commit.

Eric

> I'll leve the final call on this to Eric, but after this I suggest we
> implement a feature freeze until after we get the current code tested
> and out. Friday is a reasonable target if others agree and we have a
> chance to test this for a couple of days.
>
>> Related: while I am digging around in there, now is probably the moment
>> for me to integrate Paul Kienzle's comments on start/stop_event_loop in
>> FigureCanvasBase, etc. I am not sure there is a consensus on this. I
>> am currently thinking that the best way to integrate this, while
>> minimizing repeated code, is a mixin plus a couple of new classes,
>> FigureCanvasBaseGUI and FigureCanvasGUIAgg. These new classes would
>> inherit the mixin and the base classes without "GUI". Interactive
>> backends would then inherit these. Non-interactive backends would
>> inherit versions that throw errors from FigureBaseCanvas. Complex, but
>> this assures clean inheritance. Thoughts welcome.
>
> See my response in the original thread. I'm not sure we ever reached
> a consensus on this one, and I'm still uncomfortable with a more
> complex hierarchy. I'm willing to be convince if you and Paul and
> Gael disagree, but I have yet to see why a flat implementation will
> not work here.
>
> JDH
>
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
> Build the coolest Linux based applications with Moblin SDK & win great prizes
> Grand prize is a trip for two to an Open Source event anywhere in the world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/
> _______________________________________________
> Matplotlib-devel mailing list
> Matplotlib-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/matplotlib-devel

--
**********************************
David M. Kaplan
Charge de Recherche 1
Institut de Recherche pour le Developpement
Centre de Recherche Halieutique Mediterraneenne et Tropicale
av. Jean Monnet
B.P. 171
34203 Sete cedex
France

Phone: +33 (0)4 99 57 32 27
Fax: +33 (0)4 99 57 32 95
http://www.ur097.ird.fr/team/dkaplan/index.html
**********************************

Hey David -- thanks for these fixes. I noticed a couple of things I
want to comment on

* avoid the ternary operator, as in

            # Figure out label rotation.
            rotation,nlc = cs.calc_label_rot_and_inline(
                slc, imin, lw, lc if self.inline else [],
                self.inline_spacing )

since this requires python2.5. I replaced this, and a similar
construct in contour.py, so please make sure I did the right thing

* avoid needless *args, **kwargs usage. We do this all the time in
pylab and to a lesser extent in axes.py because we are passing the
function call on to another layer. In that case, at least document
the proper signature as the first line in the docstring. But unless
we need it, avoid it, eg in

    def start_event_loop(self,*args,**kwargs):
        """
        Start an event loop. This is used to start a blocking event
        loop so that interactive f

If you want to give the user who is using a known UI that supports
extra args, do it like

     def func(self, timeout, **kwargs):

and pass the kwargs through, but at lease require the known args and
kwargs in the main signature.

* Using an empty list in a python kwarg as the default is a gotcha, as in

    def calc_label_rot_and_inline( self, slc, ind, lw, lc=[], spacing=5 ):

The reason is that if the function mutates the list, this often leads
to unintended consequences as the list is module level and thus shared
between instances and method calls. The standard idiom for mutable
(lists and dicts) keyword args s

    def func(x=None):
        if x is None: x = []

I have fixed this in contour.py

JDH

···

On Thu, Jul 24, 2008 at 7:31 AM, David Kaplan <David.Kaplan@...622...> wrote:

I committed the changes to clabel (r5830).

Hi,

I made the suggested fixes. Comments below:

> I committed the changes to clabel (r5830).

Hey David -- thanks for these fixes. I noticed a couple of things I
want to comment on

* avoid the ternary operator, as in

            # Figure out label rotation.
            rotation,nlc = cs.calc_label_rot_and_inline(
                slc, imin, lw, lc if self.inline else [],
                self.inline_spacing )

since this requires python2.5. I replaced this, and a similar
construct in contour.py, so please make sure I did the right thing

The reason I used this was that I saw the following line in contour.py
(line 325):

lc = [tuple(l) for l in linecontour]

Doesn't this also require 2.5 or is the if different than the for?
Should this also be changed?

* avoid needless *args, **kwargs usage. We do this all the time in
pylab and to a lesser extent in axes.py because we are passing the
function call on to another layer. In that case, at least document
the proper signature as the first line in the docstring. But unless
we need it, avoid it, eg in

I removed all this arbitrary argument stuff. I was thinking that if we
ever invented a better mouse trap, this would let us automatically pass
that on to GUI's we haven't written specific functions for yet. But we
are unlikely to invent a better mouse trap.

* Using an empty list in a python kwarg as the default is a gotcha, as in

    def calc_label_rot_and_inline( self, slc, ind, lw, lc=[], spacing=5 ):

The reason is that if the function mutates the list, this often leads
to unintended consequences as the list is module level and thus shared
between instances and method calls. The standard idiom for mutable
(lists and dicts) keyword args s

    def func(x=None):
        if x is None: x = []

I have fixed this in contour.py

I don't really understand how this can be a problem, but it probably
isn't that important unless you feel like enlightening me.

Cheers,
David

···

On Thu, 2008-07-24 at 08:38 -0500, John Hunter wrote:

On Thu, Jul 24, 2008 at 7:31 AM, David Kaplan <David.Kaplan@...622...> wrote:

JDH

--
**********************************
David M. Kaplan
Charge de Recherche 1
Institut de Recherche pour le Developpement
Centre de Recherche Halieutique Mediterraneenne et Tropicale
av. Jean Monnet
B.P. 171
34203 Sete cedex
France

Phone: +33 (0)4 99 57 32 27
Fax: +33 (0)4 99 57 32 95
http://www.ur097.ird.fr/team/dkaplan/index.html
**********************************

* avoid the ternary operator, as in

            # Figure out label rotation.
            rotation,nlc = cs.calc_label_rot_and_inline(
                slc, imin, lw, lc if self.inline else [],
                self.inline_spacing )

since this requires python2.5. I replaced this, and a similar
construct in contour.py, so please make sure I did the right thing

The reason I used this was that I saw the following line in contour.py
(line 325):

lc = [tuple(l) for l in linecontour]

Doesn't this also require 2.5 or is the if different than the for?
Should this also be changed?

No, this (a list comprehension) is fine. The problem is the ternary
operator " lc if self.inline else []"
which is not supported in python 2.4. I believe all the python
2.5isms have been removed.

    def func(x=None):
        if x is None: x = []

I have fixed this in contour.py

I don't really understand how this can be a problem, but it probably
isn't that important unless you feel like enlightening me.

See for example
http://www.velocityreviews.com/forums/t350126-default-argument-to-init.html

···

On Thu, Jul 24, 2008 at 9:32 AM, David Kaplan <David.Kaplan@...622...> wrote:

David Kaplan wrote:

* Using an empty list in a python kwarg as the default is a gotcha, as in

    def calc_label_rot_and_inline( self, slc, ind, lw, lc=[], spacing=5 ):

The reason is that if the function mutates the list, this often leads
to unintended consequences as the list is module level and thus shared
between instances and method calls. The standard idiom for mutable
(lists and dicts) keyword args s

    def func(x=None):
        if x is None: x = []

I have fixed this in contour.py

I don't really understand how this can be a problem, but it probably
isn't that important unless you feel like enlightening me.

The default value for the function is created when the interpreter executes the define statement. Thus, if you were to do something that modifies the list inplace, like append(), the default argument will retain those changes. For instance, try this:

def foo(l=[]):
     l.append('foo')
     print l
foo()
foo(['bar'])
foo()

Ryan

···

--
Ryan May
Graduate Research Assistant
School of Meteorology
University of Oklahoma