Use of meta on Artist

I’ve just been reviewing a really useful PR (https://github.com/matplotlib/matplotlib/pull/1531) from Pierre Haessig which speeds up the drawing of non-visible artists by bringing the following line to the top of the LineArtist’s draw method:

if self.get_visible() is False:
    return

This does fix the problem (and will fix the problem for all other artists if applied in the same way), but it relies on a developer remembering the rule of thumb that they must always start their draw method with these two (simple) lines. Additionally, testing this functionality is actually quite hard without resorting to timing the execution.

It made we wonder if there was a better approach to fixing this. Having a decorator to do this for you is a good idea, except that a developer would need to remember to decorate their subclass’ draw method, so the next level up is to use a metaclass to always wrap the draw method with the “if visible” lines.

An example of implementing this (apologies if the code doesn’t come out well in the email):

class ArtistMeta(type):
def new(cls, classname, bases, class_dict):
# replace the draw method with one which short-circuits if self.visible is False
draw_method = class_dict[‘draw’]
def draw(self, *args, **kwargs):
if self.visible is False:
print ‘draw not called with visible={}’.format(self.visible)
return
else:
return draw_method(self, *args, **kwargs)
class_dict[‘draw’] = draw

    return  type.__new__(cls, classname, bases, class_dict)

class Artist(object):
metaclass = ArtistMeta

def __init__(self, visible=True):
    self.visible = visible
   
def draw(self, renderer=None):
    print 'draw called with visible={}'.format(self.visible)
    return 'foobar'

class SubArtist(Artist):
def draw(self, renderer=None):
print “subclass’ draw method”
return Artist.draw(self, renderer=renderer)

With the following results:

a = Artist().draw(‘wibble’)
draw called with visible=True

b = Artist(False).draw(‘wibble’)
draw not called with visible=False

c = SubArtist(True).draw(‘wibble’)
subclass’ draw method
draw called with visible=True

d = SubArtist(False).draw(‘wibble’)
draw not called with visible=False

In my eyes this makes testing the functionality possible without timing (and is therefore an improvement), but I wanted to know how others felt about the approach, and in particular, using more metaclasses in matplotlib (a simple tutorial which I found useful: http://www.voidspace.org.uk/python/articles/metaclasses.shtml).

Cheers,

Phil

The problem is that I don't think we can do this for all artists. Some may need to create groupings, or push and pop state even if they are "invisible". For instance, this is used in the SVG backend to create named groupings (possibly empty) that are referenced from Javascript to provide interactivity. I think I'd rather keep this to the contained solution in the PR and not try to generalize it beyond that.

If we did want to generalize, this would only apply to "leaf node" artists, and not artists that simply exist to contain other artists -- and conceivably we could implement that using either a decorator or explicit chaining to a base class, but in any event it would have to be a manual process to determine which artists this would apply to. We could insert a class in the heirarchy of "ConcreteArtist" (or somesuch) to handle this.

Mike

···

On 11/26/2012 06:17 AM, Phil Elson wrote:

I've just been reviewing a really useful PR (https://github.com/matplotlib/matplotlib/pull/1531) from Pierre Haessig which speeds up the drawing of non-visible artists by bringing the following line to the top of the LineArtist's draw method:

    if self.get_visible() is False:
        return

This *does* fix the problem (and will fix the problem for all other artists if applied in the same way), but it relies on a developer remembering the rule of thumb that they must always start their draw method with these two (simple) lines. Additionally, testing this functionality is actually quite hard without resorting to timing the execution.

It made we wonder if there was a better approach to fixing this. Having a decorator to do this for you is a good idea, except that a developer would need to remember to decorate their subclass' draw method, so the next level up is to use a metaclass to *always* wrap the draw method with the "if visible" lines.

An example of implementing this (apologies if the code doesn't come out well in the email):

class ArtistMeta(type):
    def __new__(cls, classname, bases, class_dict):
        # replace the draw method with one which short-circuits if self.visible is False
        draw_method = class_dict['draw']
        def draw(self, *args, **kwargs):
            if self.visible is False:
                print 'draw **not** called with visible={}'.format(self.visible)
                return
            else:
                return draw_method(self, *args, **kwargs)
        class_dict['draw'] = draw

        return type.__new__(cls, classname, bases, class_dict)

class Artist(object):
    __metaclass__ = ArtistMeta

    def __init__(self, visible=True):
        self.visible = visible

    def draw(self, renderer=None):
        print 'draw called with visible={}'.format(self.visible)
        return 'foobar'

class SubArtist(Artist):
    def draw(self, renderer=None):
        print "subclass' draw method"
        return Artist.draw(self, renderer=renderer)

With the following results:

>>> a = Artist().draw('wibble')
draw called with visible=True

>>> b = Artist(False).draw('wibble')
draw **not** called with visible=False

>>> c = SubArtist(True).draw('wibble')
subclass' draw method
draw called with visible=True

>>> d = SubArtist(False).draw('wibble')
draw **not** called with visible=False

In my eyes this makes testing the functionality possible without timing (and is therefore an improvement), but I wanted to know how others felt about the approach, and in particular, using more metaclasses in matplotlib (a simple tutorial which I found useful: http://www.voidspace.org.uk/python/articles/metaclasses.shtml).

Cheers,

Phil

------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/matplotlib-devel

The problem is that I don't think we can do this for all artists. Some
may need to create groupings, or push and pop state even if they are
"invisible". For instance, this is used in the SVG backend to create
named groupings (possibly empty) that are referenced from Javascript to
provide interactivity. I think I'd rather keep this to the contained
solution in the PR and not try to generalize it beyond that.

If we did want to generalize, this would only apply to "leaf node"
artists, and not artists that simply exist to contain other artists --
and conceivably we could implement that using either a decorator or
explicit chaining to a base class, but in any event it would have to be
a manual process to determine which artists this would apply to. We
could insert a class in the heirarchy of "ConcreteArtist" (or somesuch)
to handle this.

I think we should be rather conservative about this sort of thing. Sometimes it is better to just explicitly put the two lines in each method than to come up with machinery to do it for you. Each level of depth in an inheritance hierarchy or "meta" chain is an additional level of complexity for someone reading the code. And if someone forgets to put in those lines, the penalty is typically from small to nil; but if they are put in automatically by fancy methods, and they are not really wanted or something else goes wrong, it can make debugging painful.

Eric

···

On 2012/11/26 7:12 AM, Michael Droettboom wrote:

Mike

On 11/26/2012 06:17 AM, Phil Elson wrote:

I've just been reviewing a really useful PR
(https://github.com/matplotlib/matplotlib/pull/1531) from Pierre
Haessig which speeds up the drawing of non-visible artists by bringing
the following line to the top of the LineArtist's draw method:

    if self.get_visible() is False:
        return

This *does* fix the problem (and will fix the problem for all other
artists if applied in the same way), but it relies on a developer
remembering the rule of thumb that they must always start their draw
method with these two (simple) lines. Additionally, testing this
functionality is actually quite hard without resorting to timing the
execution.

It made we wonder if there was a better approach to fixing this.
Having a decorator to do this for you is a good idea, except that a
developer would need to remember to decorate their subclass' draw
method, so the next level up is to use a metaclass to *always* wrap
the draw method with the "if visible" lines.

An example of implementing this (apologies if the code doesn't come
out well in the email):

class ArtistMeta(type):
    def __new__(cls, classname, bases, class_dict):
        # replace the draw method with one which short-circuits if
self.visible is False
        draw_method = class_dict['draw']
        def draw(self, *args, **kwargs):
            if self.visible is False:
                print 'draw **not** called with
visible={}'.format(self.visible)
                return
            else:
                return draw_method(self, *args, **kwargs)
        class_dict['draw'] = draw

        return type.__new__(cls, classname, bases, class_dict)

class Artist(object):
    __metaclass__ = ArtistMeta

    def __init__(self, visible=True):
        self.visible = visible

    def draw(self, renderer=None):
        print 'draw called with visible={}'.format(self.visible)
        return 'foobar'

class SubArtist(Artist):
    def draw(self, renderer=None):
        print "subclass' draw method"
        return Artist.draw(self, renderer=renderer)

With the following results:

>>> a = Artist().draw('wibble')
draw called with visible=True

>>> b = Artist(False).draw('wibble')
draw **not** called with visible=False

>>> c = SubArtist(True).draw('wibble')
subclass' draw method
draw called with visible=True

>>> d = SubArtist(False).draw('wibble')
draw **not** called with visible=False

In my eyes this makes testing the functionality possible without
timing (and is therefore an improvement), but I wanted to know how
others felt about the approach, and in particular, using more
metaclasses in matplotlib (a simple tutorial which I found useful:
http://www.voidspace.org.uk/python/articles/metaclasses.shtml).

Cheers,

Phil

I think you and Mike are skirting around a key point here. You can always
add the line if you need it, but if you don't need it (or can't use it), by
use of a metaclass, there's no way to "opt out" so to speak.

I'll also add that we don't need to add any more indirection (i.e. another
Python function call) to our drawing stack--we really need to be doing
everything possible to take every last millisecond out of the call to
draw().

Ryan

···

On Mon, Nov 26, 2012 at 12:23 PM, Eric Firing <efiring@...229...> wrote:

On 2012/11/26 7:12 AM, Michael Droettboom wrote:
> The problem is that I don't think we can do this for all artists. Some
> may need to create groupings, or push and pop state even if they are
> "invisible". For instance, this is used in the SVG backend to create
> named groupings (possibly empty) that are referenced from Javascript to
> provide interactivity. I think I'd rather keep this to the contained
> solution in the PR and not try to generalize it beyond that.
>
> If we did want to generalize, this would only apply to "leaf node"
> artists, and not artists that simply exist to contain other artists --
> and conceivably we could implement that using either a decorator or
> explicit chaining to a base class, but in any event it would have to be
> a manual process to determine which artists this would apply to. We
> could insert a class in the heirarchy of "ConcreteArtist" (or somesuch)
> to handle this.

I think we should be rather conservative about this sort of thing.
Sometimes it is better to just explicitly put the two lines in each
method than to come up with machinery to do it for you. Each level of
depth in an inheritance hierarchy or "meta" chain is an additional level
of complexity for someone reading the code. And if someone forgets to
put in those lines, the penalty is typically from small to nil; but if
they are put in automatically by fancy methods, and they are not really
wanted or something else goes wrong, it can make debugging painful.

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

Note that we already use a decorator for a similar purpose (allow_rasterization).

Also, please note that the “draw” method is not just for drawing things. There are other things being done within the draw method, and I think some of them still need to be done even though the artist is invisible.

My personal inclination on this issue is to refactor the “draw” method, the only method being called during the drawing time. But, yes there are sideeffects.

Regards,

-JJ

···

On Tue, Nov 27, 2012 at 3:40 AM, Ryan May <rmay31@…149…> wrote:

On Mon, Nov 26, 2012 at 12:23 PM, Eric Firing <efiring@…229…> wrote:


Monitor your physical, virtual and cloud infrastructure from a single

web console. Get in-depth insight into apps, servers, databases, vmware,

SAP, cloud infrastructure, etc. Download 30-day Free Trial.

Pricing starts from $795 for 25 servers or applications!

http://p.sf.net/sfu/zoho_dev2dev_nov


Matplotlib-devel mailing list

Matplotlib-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/matplotlib-devel

On 2012/11/26 7:12 AM, Michael Droettboom wrote:

The problem is that I don’t think we can do this for all artists. Some

may need to create groupings, or push and pop state even if they are

“invisible”. For instance, this is used in the SVG backend to create

named groupings (possibly empty) that are referenced from Javascript to

provide interactivity. I think I’d rather keep this to the contained

solution in the PR and not try to generalize it beyond that.

If we did want to generalize, this would only apply to “leaf node”

artists, and not artists that simply exist to contain other artists –

and conceivably we could implement that using either a decorator or

explicit chaining to a base class, but in any event it would have to be

a manual process to determine which artists this would apply to. We

could insert a class in the heirarchy of “ConcreteArtist” (or somesuch)

to handle this.

I think we should be rather conservative about this sort of thing.

Sometimes it is better to just explicitly put the two lines in each

method than to come up with machinery to do it for you. Each level of

depth in an inheritance hierarchy or “meta” chain is an additional level

of complexity for someone reading the code. And if someone forgets to

put in those lines, the penalty is typically from small to nil; but if

they are put in automatically by fancy methods, and they are not really

wanted or something else goes wrong, it can make debugging painful.

I think you and Mike are skirting around a key point here. You can always add the line if you need it, but if you don’t need it (or can’t use it), by use of a metaclass, there’s no way to “opt out” so to speak.

I’ll also add that we don’t need to add any more indirection (i.e. another Python function call) to our drawing stack–we really need to be doing everything possible to take every last millisecond out of the call to draw().

Ryan

Ryan May
Graduate Research Assistant
School of Meteorology
University of Oklahoma