patch for adding manual label location selection to clabel

Development matplotlib-devel
1

Hi all,

I committed to svn (revision 5782) a version of the patch for clabel and
waitforbuttonpress. I haven't perfected label rotation yet, but it
works at the moment. I also haven't yet followed Paul Kienzle's
suggestions (though I think they are a good idea), as I wanted to get a
bit more information in relation to one of Gael's comments below.

My responses to Gael's comments are mixed in below.

Cheers,
David

OK, a few comment from quickly glancing at the patch:

* What happens if twe are in a non interactive terminal, such as
  postscript? If this thing is running on a headless server, we don't
  want to hang the script because the manual option has been selected.

The current answer is don't do that. In my opinion, all of these
functions should probably return an error if not in an interactive
terminal or graphical backend. I looked around a bit for a backend
neutral way to ask that question, but didn't find anything.
isinteractive seems like it should be relevant, but for Agg backend,
which doesn't display a graphical window, it was true, so this doesn't
seem to fit the bill. I imagine there is a way and one of you can tell
me what it is. If so, I will add it in the right place.

Another option would be to create a start_event_loop function like Paul
suggested and overload that function in those backends that aren't
interactive so that it returns an error, but this requires writing one
such function for each non-interactive backend. Also, is there any case
where an event loop would be useful for a graphically non-interactive
backend? If so, this would again mean that this problem would be easier
to deal with once in the Blocking* classes.

* Putting this argument in "*args" seems like a bad style (it looks like
  matlab). Why don't you use a "label_pos='auto'" keyword argument. This
  would be much more robust to the addition of other options, and more in
  agreement with the rest of the style of pylab's arguments.

Originally I intended to allow either the v argument or manual, but both
ended up hanging around - think of it as a comfort food for matlab
users. But you're right and I have now placed it in a keyword argument
"manual".

···

On Thu, 2008-07-17 at 17:19 +0200, Gael Varoquaux wrote:

I have to run. I haven't reviewed the patch very well. I think you should
address those two comments and send it again to the list for review.
You'll probably get useful advice and maybe learn more about Python.

Thanks,

Gaël

--
**********************************
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
**********************************

2

Just reading through the blocking_inpu with comments mostly unrelated
to those you are raising here, and I was just reading for style rather
than logic. Some of this stuff may not be your code but here is what
I found:

    def __init__(self, fig, eventslist=()):
        self.fig = fig
        assert isinstance(eventslist, tuple), "Requires a tuple of
event name strings"
        self.eventslist = eventslist

It is rarely necessary to require *a tuple* though in some cases it
might be. Wouldn't a list work here? We use duck typing in mpl: eg
if you want to make sure the input is iterable and it contains strings

  import matplotlib.cbook

  if not cbook.iterable(eventslist):
      raise ValueError('events list must be iterable')

  if cbook.is_string_like(eventslist):
      raise ValueError('eventslist cannot be a string')

  for event in eventslist:
      if not cbook.is_string_like(event):
          raise ValueError('events list must be a list of strings')

I would probably write a cbook method is_sequence_of_strings and just
call that since it will be more readable and reusable...

I notice there are some residual print statements in the code -- these
should be replaced by the verbose handler in matplotlib/__init__.py,
eg in

                if timeout > 0 and counter > timeout/0.01:
                    print "Timeout reached";
                    break;

and
            if self.verbose:
                print "input %i: %f,%f" % (len(self.clicks),
                                    event.xdata, event.ydata)

and others in contour.py

You can replace these with

  import matplotlib
  matplotlib.verbose.report('something')

and

  matplotlib.verbose.report('some nitty gritty details', 'debug')

There should be no prints in mpl.

In contour.py, we have

        xmax = np.amax(np.array(linecontour)[:,0])
        xmin = np.amin(np.array(linecontour)[:,0])
        ymax = np.amax(np.array(linecontour)[:,1])
        ymin = np.amin(np.array(linecontour)[:,1])

which needlessly repears the array creation. I would create it once
and reuse it.

That's all for now. Thanks.

JDH

···

On Thu, Jul 17, 2008 at 2:44 PM, David M. Kaplan <David.Kaplan@...622...> wrote:

Hi all,

I committed to svn (revision 5782) a version of the patch for clabel and
waitforbuttonpress. I haven't perfected label rotation yet, but it
works at the moment. I also haven't yet followed Paul Kienzle's
suggestions (though I think they are a good idea), as I wanted to get a
bit more information in relation to one of Gael's comments below.

3

Hi,

I just committed some changes to deal with these comments. Responses
below.

Cheers, David

    def __init__(self, fig, eventslist=()):
        self.fig = fig
        assert isinstance(eventslist, tuple), "Requires a tuple of
event name strings"
        self.eventslist = eventslist

I would probably write a cbook method is_sequence_of_strings and just
call that since it will be more readable and reusable...

Method added to cbook.

I notice there are some residual print statements in the code -- these
should be replaced by the verbose handler in matplotlib/__init__.py,
eg in

                if timeout > 0 and counter > timeout/0.01:
                    print "Timeout reached";
                    break;

and
            if self.verbose:
                print "input %i: %f,%f" % (len(self.clicks),
                                    event.xdata, event.ydata)

and others in contour.py

You can replace these with

  import matplotlib
  matplotlib.verbose.report('something')

and

  matplotlib.verbose.report('some nitty gritty details', 'debug')

There should be no prints in mpl.

Fixed.

In contour.py, we have

        xmax = np.amax(np.array(linecontour)[:,0])
        xmin = np.amin(np.array(linecontour)[:,0])
        ymax = np.amax(np.array(linecontour)[:,1])
        ymin = np.amin(np.array(linecontour)[:,1])

I noticed these previously, but didn't touch them as this is in the part
of the code that is somewhat mysterious to me. linecontour should
always be an array (at least the linecontours in a Path), so I don't see
much point in forcing it to array. I have removed these and several
other np.array or np.asarray calls that seemed extraneous to me. If
desired, I can include a single np.array call, but I don't think it is
required.

Thanks,
David

···

On Thu, 2008-07-17 at 15:16 -0500, John Hunter wrote:

--
**********************************
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
**********************************

4

This method will return true on a string, which is probably not what you want

  In [46]: is_sequence_of_strings('jdh was here')
  Out[46]: True

My original example included an additional check on is_string_like(obj).

If you want to optionally support a string as a sequence of strings, I
think we might need a kwarg to make this explicit.

JDH

···

On Fri, Jul 18, 2008 at 3:50 AM, David M. Kaplan <David.Kaplan@...622...> wrote:

I would probably write a cbook method is_sequence_of_strings and just
call that since it will be more readable and reusable...

Method added to cbook.

5

Hi,

My bad - I forgot strings are iterable. This should now be fixed.

Cheers,
David

···

On Fri, 2008-07-18 at 09:41 -0500, John Hunter wrote:

On Fri, Jul 18, 2008 at 3:50 AM, David M. Kaplan <David.Kaplan@...622...> wrote:

>> I would probably write a cbook method is_sequence_of_strings and just
>> call that since it will be more readable and reusable...
>>
>
> Method added to cbook.

This method will return true on a string, which is probably not what you want

  In [46]: is_sequence_of_strings('jdh was here')
  Out[46]: True

My original example included an additional check on is_string_like(obj).

If you want to optionally support a string as a sequence of strings, I
think we might need a kwarg to make this explicit.

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
**********************************