check `ret == False` in Timer._on_timer #1493

minrk · 2012-11-13T19:58:11Z

The docstring states:

Functions can return False if they should not be called any more.

Cleanup commit 2f11dee changed this logic so any return value whose boolean interpretation is False (e.g. None) would be unregistered.

This restores the logic to check only for explicit False (not None, not 0, etc.).

fixes #1492

The docstring states: > Functions can return False if they should not be called any more. Cleanup commit 2f11dee changed this logic, so any return value whose boolean interpretation is False (e.g. None) would be unregistered. fixes #1492

minrk · 2012-11-13T20:01:07Z

I don't know how to write a timer test for matplotlib, but it should simply confirm that a callback that returns various values don't get unregistered after one call (None, 0, 1, True, '', [], etc.).

WeatherGod · 2012-11-13T20:04:41Z

lib/matplotlib/backend_bases.py

@@ -1123,7 +1123,7 @@ def _on_timer(self):
        '''
        for func, args, kwargs in self.callbacks:
            ret = func(*args, **kwargs)
-            if not ret:
+            if ret is False:


We might want to add a comment here stating that it has to be this form and why. This will prevent future devs from "fixing" this odd semantic.

I would think the docstring three lines above is good enough, but perhaps it is not.

Considering that the change that broke everything came about when we did the PEP8 changes, the docstrings wouldn't have been visible in the github diffs and so we didn't notice it then. Better safe than sorry.

Of course, I would have to wonder about the logic of depending on the return value of an arbitrary callback function to determine if it is to be removed or not...

comment added

This change is not clear at all, and should be commented.

I'm guessing you want to avoid the case were ret is an empty list, or an integer equal to 0 etc... ie, all the case were ret isn't a boolean, but evaluates to false.

That method is tricky and smelly: I wouldn't expect that in a python code.

mdboom · 2012-11-13T20:24:29Z

Given the severity of #1492, It would probably make sense to make sure this gets onto the v1.2.x branch

jenshnielsen · 2012-11-13T20:54:25Z

@mdboom I don't think this is on 1.2.x. It is part of #1345 which was never merged into 1.2.x and the same comit that caused the bbox_tight isssue fixed by #1420 which was not present in 1.2.x at any time

NelleV · 2012-11-13T21:19:35Z

lib/matplotlib/backend_bases.py

@@ -1123,7 +1123,9 @@ def _on_timer(self):
        '''
        for func, args, kwargs in self.callbacks:
            ret = func(*args, **kwargs)
-            if not ret:
+            # docstring above explains why we use `if ret is False` here,
+            # instead of `if not ret`.


The comment doesn't really help, nor does the docstring. This code is not very pythonic (I would guess this was written by someone who comes from , and I think a good explanation is necessary in case of more code "cleanup".

Also, this code will break if the function called returns the integer 0 (if that is ever the case..)

My 2 cents,
N

minrk · 2012-11-15T01:27:49Z

The comment doesn't really help, nor does the docstring.

From the docstring:

Functions can return False if they should not be called any more.

What would be more clear? Perhaps:

If, at any point, a callback returns False (and only False), it will no longer be called.

or

A callback can return False (and only False) to signal that its last call has been made, and it should not be called anymore.

or, for ret == False semantics:

If a callback returns 0 or False, it will no longer be called.

Also, this code will break if the function called returns the integer 0 (if that is ever the case..)

You mean it will behave as described in the docstring? False is described as a special case, so I would not expect 0 to trigger that event. If 0 should trigger it, the docstring should be updated to indicate that. Differentiating between False and 0 is uncommon, so perhaps not the best API. I was only trying to fix the code so it behaved as the API is described.

Here's a question: Does anyone actually use this return False to stop behavior? I certainly had no idea it was there before fixing this bug. Perhaps a more Pythonic approach would be to raise a special Exception class, or return a more specialized termination value, rather than False.

NelleV · 2012-11-15T09:27:25Z

My mistake: what I meant is that the current "fix" does not behave as the old code was (before my change that breaks everything). A boolean in CPython inherits from a integer, so the difference is very little:

ret = False; ret == 0 returns True
ret = False, ret is 0 returns False

Also, this relies on a CPython implementation detail (False is a singleton): this may break in future version of CPython (but is highly unlikely) or another python implementation (I don't know anything about those).
This is why developpers should not use "ret is True" or "ret is False" to check for boolean values. Quoting PEP8

Yes: if greeting:
No: if greeting == True:
Worse: if greeting is True:

I think indeed, raising an Exception is a better way to do that, but I think it is out of the scope of this PR. Maybe we can open a ticket (and a discussion on the mailing list).

per review

minrk · 2012-11-15T18:55:25Z

I've changed the code to ret == False, and updated the docstring so it actually describes this case.

A future Issue can address how to do it better with an Exception, deprecation plan, etc.

dmcdougall · 2012-11-26T15:59:12Z

Is everybody happy with this?

WeatherGod · 2012-11-26T16:02:05Z

I would personally like @dopplershift opinion because I think he wrote the original logic.

minrk · 2012-11-27T00:21:43Z

I should note that the current logic is now exactly the same as it was prior to the pep8 pass that caused the problem. The docstring now reflects that.

Fix coercion of values to False in _on_timer() introduced in PEP8 clean-up. Fixes #1492.

dopplershift · 2012-11-27T03:19:54Z

Yup, I'm the one to thank/blame/set fire to for the current architecture of the timers (I am both glad and surprised to see their use outside of animations).

The return False as a way for the callback to end itself started because that's how [Py]Gtk decides whether a timer should repeat--using the return of the callback (and really, that's inherited from the C toolkit). As the design evolved and was generalized across backends, this use of return True/False was no longer essential, but I never revisited that mechanism--it made perfect sense at the time from my gtk-warped point of view. (Currently, the only thing that must return True/False is the _on_timer() method of the GtkTimer.)

I agree that an exception would make more sense from a pure Python standpoint, but at this point we have a published API that we don't want to break (at least IPython is using it apparently) and I don't think the "wart" of the return True/False is remotely big enough to make me want to change it. (I mean, it did work perfectly well until the PEP 8 "fix" ...)

The fix looks good, so I'm merging. Thanks @minrk (and thanks @WeatherGod for bringing this to my attention).

NelleV · 2012-11-27T17:18:00Z

I have to disagree on many points:

First, not only this is a unpythonic code: it is code that breaks very easily due to the nature of python. Also, it did not work before: it just didn't fail, and @minrk updated the docstring to specify the case of a null integer which was not mentioned before (and I'm not sure you spotted the fact that returning a 0 would behave like returning False :) ).

I don't think because a API is public (is it really public ?), it should never change. Sure, it is best not to change the APIs all the time, but when there is a problem with one, better fix it sooner than later. Also, what @minrk mentions isn't that ipython uses the API: it's just a bug that is underlined when using interactive tools such as ipython. Hence, I think we should make the change (I can do the patch).

You may think I am nitpicking here, but the few talks I had with very experience python programmers (CPython core dev in fact) convinced me we should not leave the code as it is right now.

I don't know the code very well, but it doesn't seem like a big change to implement (whatever the option we choose to implement), and I think it would be worth it to quickly do a patch to measure how much lines of code it would involve.

dopplershift · 2012-11-27T18:27:05Z

The API is public in terms of the fact that the documented behavior (that a user of the timer classes would rely upon) was that a callback returning False (or 0) causes the callback to cease to be called. IPython is using the API (though not this behavior), as they use our TimerMac class--which is why they reported the bug (matplotlib itself only uses the timers for animations).

The PEP8 change was the only thing broken, as it made the code suddenly consider a return value of None (which happens in the cases where there is no return) the same as False--the original code did not intend this (and is why it wasn't written that way in the first place). I'm really fuzzy on how this will break "easily" in the future given that the requirement of == False is now better documented. This broke because semantically different code was committed as part of formatting changes and I've found it perfectly acceptable, and even needed, that False and None are handled completely differently. We could make it slightly more pythonic by the following (slight API change):

if not ret and ret is not None:

However, I do agree that the exception is the superior solution--I just think the case against the current code is overblown.

If somebody cares enough to change this to an exception, then have at it. The steps would be:

Make it issue a deprecation warning (for returns == False) for the next release or two
Add new exception-based code for next release
Come back and remove the old behavior in a later release
Document the API change

(I believe that's been our policy previously for changes to API.) I lack the time and motivation to do this, but pledge to review a PR that does.

NelleV · 2012-11-27T21:10:05Z

I can do a PR when I have the time if everyone agrees that raising an exception is the way to go.

check ret is False in Timer._on_timer

c5304ee

The docstring states: > Functions can return False if they should not be called any more. Cleanup commit 2f11dee changed this logic, so any return value whose boolean interpretation is False (e.g. None) would be unregistered. fixes #1492

minrk mentioned this pull request Nov 13, 2012

MacOSX backend blocks in IPython QtConsole #1492

Closed

WeatherGod reviewed Nov 13, 2012
View reviewed changes

add comment to protect against future 'cleanup' of if ret is False

2ac78c7

NelleV reviewed Nov 13, 2012
View reviewed changes

use ret == False instead of ret is False

73371b0

per review

ghost assigned dopplershift Nov 27, 2012

dopplershift added a commit that referenced this pull request Nov 27, 2012

Merge pull request #1493 from minrk/retFalse

0da885a

Fix coercion of values to False in _on_timer() introduced in PEP8 clean-up. Fixes #1492.

dopplershift merged commit 0da885a into matplotlib:master Nov 27, 2012

Uh oh!

check ret == False in Timer._on_timer #1493

check ret == False in Timer._on_timer #1493

Uh oh!

Conversation

minrk commented Nov 13, 2012

Uh oh!

minrk commented Nov 13, 2012

Uh oh!

WeatherGod Nov 13, 2012

Choose a reason for hiding this comment

Uh oh!

minrk Nov 13, 2012

Choose a reason for hiding this comment

Uh oh!

WeatherGod Nov 13, 2012

Choose a reason for hiding this comment

Uh oh!

minrk Nov 13, 2012

Choose a reason for hiding this comment

Uh oh!

NelleV Nov 13, 2012

Choose a reason for hiding this comment

Uh oh!

mdboom commented Nov 13, 2012

Uh oh!

jenshnielsen commented Nov 13, 2012

Uh oh!

NelleV Nov 13, 2012

Choose a reason for hiding this comment

Uh oh!

minrk commented Nov 15, 2012

Uh oh!

NelleV commented Nov 15, 2012

Uh oh!

minrk commented Nov 15, 2012

Uh oh!

dmcdougall commented Nov 26, 2012

Uh oh!

WeatherGod commented Nov 26, 2012

Uh oh!

minrk commented Nov 27, 2012

Uh oh!

dopplershift commented Nov 27, 2012

Uh oh!

NelleV commented Nov 27, 2012

Uh oh!

dopplershift commented Nov 27, 2012

Uh oh!

NelleV commented Nov 27, 2012

Uh oh!

Uh oh!

check `ret == False` in Timer._on_timer #1493

check `ret == False` in Timer._on_timer #1493