On Fri, May 15, 2009 at 5:16 PM, Jae-Joon Lee <lee.j.j...@gmail.com> wrote:
> Attached is a patch that I want to commit to the trunk.
> It introduces new attribute "_check_contains" (any name suggestion?)
I'm not keen on the name either -- something more explicit would be
better. Maybe, set_annotation_clip or something like that, which is
more descriptive to me.
> for Annotation class.
>
> * True : the annotation will only be drawn when self.xy is
> inside the axes.
> * False : the annotation will always be drawn regardless of
> its position.
> * None : the self.xy will be checked only if *xycoords* is "data"
>
> The default value is None, i.e., position is only checked if the
> xycoords is "data".
>
> I'll commit this soon if others don't object.
You don't need permission :-) but I took a look at the patch. Inline
comments below
+ def get_check_contains(self):
+ " retrun *check_contains* attribute. "
+ return self._check_contains
+
retrun is misspelled
+
+
def update_positions(self, renderer):
+ xy_pixel = self._get_position_xy(renderer)
+ self._update_position_xytext(renderer, xy_pixel)
+
+
+ def _get_position_xy(self, renderer):
+ x, y = self.xy
+ return self._get_xy(x, y, self.xycoords)
+
When you are extending/fixing existing code and come across methods
with no docs, could you write a one or two line doc string for them?
I wrote many of these and at the time they were so obvious that they
didn't need docstrings, but as time passes and I reencounter them, I
wish there was a simple line explaining them. As you are digging
through the code figuring them all out, it is a great time to drop in
a simple one-liner docstring (eg explaining what coord system is being
returned by _get_position_xy). As the famous coding quip says, the
literal wording or author of which I cannot dig up right now, "Leave
comments in your code -- someone may read it someday, and that someone
may be you!"
+ class _SimpleEvent:
+ def __init__(self, xy):
+ self.x, self.y = xy
+
+ def _check_xy(self, renderer, xy_pixel):
+ "given the xy coordinate, check if the annotation need to be drawn"
+
+ b = self.get_check_contains()
+ if b or (b is None and self.xycoords == "data"):
+ # check if self.xy is inside the axes.
+ if not self.axes.contains(Annotation._SimpleEvent(xy_pixel))[0]:
+ return False
+
I'm not wild about this -- I find myself doing similar hacks when
writing GUIs and doing other stuff where the quick-and-dirty is easier
than the right way, but for mpl, which is a library and is growing
rapidly and has already reached the point where no one developer has
their head around the whole thing, I think we need to be careful about
these kinds of hacks that can result in subtle bugs down the road.
You are relying on the event infrastructure API via the Axes.contain
method, but are hacking around it by just providing the minimal set of
things that are needed for the contains call with the _SimpleEvent.
The problem is, if the event API or the contains method changes later,
this is a latent bug which is difficult to unit test since it is
primarily exposed interactively. Axes.contains expects a MouseEvent
according to the signature -- I haven't dug to deeply here but with a
little more work can we provide what it is expecting rather than the
stripped down proxy class (and does "contains" really need a
MouseEvent or would a LocationEvent would suffice -- this may be
simply a matter of renaming the arg to contains....). Or perhaps the
Axes.contains method itself is misguided, requiring a full-fledged
event when an x/y location would suffice. If it is fairly easy, I
would like to fix this here and now rather than hack around a bad
design decision.
Thanks for the patch!
JDH
------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables
unlimited royalty-free distribution of the report engine
for externally facing server and web deployment.
http://p.sf.net/sfu/businessobjects
_______________________________________________
Matplotlib-users mailing list
Matplotlib-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/matplotlib-users
