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

Merged
merged 3 commits into from Nov 27, 2012

Projects

None yet

7 participants

@minrk
minrk commented Nov 13, 2012

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

@minrk minrk check `ret is False` in Timer._on_timer
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
c5304ee
@minrk
minrk commented Nov 13, 2012

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 WeatherGod and 2 others commented on an outdated diff Nov 13, 2012
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:
@WeatherGod
WeatherGod Nov 13, 2012 Matplotlib Developers member

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.

@minrk
minrk Nov 13, 2012

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

@WeatherGod
WeatherGod Nov 13, 2012 Matplotlib Developers member

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

@minrk
minrk Nov 13, 2012

comment added

@NelleV
NelleV Nov 13, 2012 collaborator

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
Member
mdboom commented Nov 13, 2012

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

@jenshnielsen
Member

@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 NelleV commented on an outdated diff Nov 13, 2012
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`.
@NelleV
NelleV Nov 13, 2012 collaborator

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
minrk commented Nov 15, 2012

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
Collaborator
NelleV commented Nov 15, 2012

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).

@minrk
minrk commented Nov 15, 2012

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
Member

Is everybody happy with this?

@WeatherGod
Member

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

@minrk
minrk commented Nov 27, 2012

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.

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

1 check passed

Details default The Travis build passed
@dopplershift
Collaborator

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
Collaborator
NelleV commented Nov 27, 2012

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
Collaborator

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:

  1. Make it issue a deprecation warning (for returns == False) for the next release or two
  2. Add new exception-based code for next release
  3. Come back and remove the old behavior in a later release
  4. 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
Collaborator
NelleV commented Nov 27, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment