Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integer division has different semantics in Python 2 and 3 -- only for the numpy codegen target #815

Closed
mstimberg opened this issue Feb 16, 2017 · 1 comment

Comments

@mstimberg
Copy link
Member

It should not come as a surprise, but I never actually thought about it in detail: we have some ugly inconsistency for integer division between Python 2 and 3. At the first sight, this could be ok because after all Python 2 and 3 do have different semantics in that regard, so this could be reflected in our interpretation of the code. The problem is that the semantics in C code are always "Python 2"-like and too complicate further, C-based runtime simulations by default use numpy for assigning to variables (to avoid the compilation overhead for trivial computation).
For example, consider the following code:

from brian2 import *

G = NeuronGroup(10, '''x : 1
                       y : 1''')
G.x = 'i / 10'  # this uses numpy by default (prefs.codegen.string_expression_target)
G.run_regularly('y = i / 10')  # this uses the standard codegen target
run(defaultclock.dt)
print(G.x)
print(G.y)

In Python 2, regardless of the codegen target, G.x and G.y will always be 0 (floor division). In Python 3 however, you'll get this for prefs.codegen.target = 'numpy':

<neurongroup.x: array([ 0. ,  0.1,  0.2,  0.3,  0.4,  0.5,  0.6,  0.7,  0.8,  0.9])>
<neurongroup.y: array([ 0. ,  0.1,  0.2,  0.3,  0.4,  0.5,  0.6,  0.7,  0.8,  0.9])>

(floating point division: Python3-semantics)
You'll get this for prefs.codegen.target = 'cython' and prefs.codegen.string_expression_target = 'cython':

<neurongroup.x: array([ 0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.])>
<neurongroup.y: array([ 0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.])>

(floor division: C semantics)
And, probably worst of all, you'll get this if you don't set any preference and have a working Cython installation (i.e. prefs.codegen.target will be 'auto' and use Cython, prefs.codgen.string_expression_target will be at its default 'numpy'):

<neurongroup.x: array([ 0. ,  0.1,  0.2,  0.3,  0.4,  0.5,  0.6,  0.7,  0.8,  0.9])>
<neurongroup.y: array([ 0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.])>

(floating point division for the assignment: Python3-semantics; floor division for the run_regularly: C semantics).

I don't see any quick fix here, I guess we have to deal with the division semantics explicitly when parsing the abstract code. Given this, I think I'd prefer to have the same semantics between Brian under Python 2 and Python 3 instead of making the semantics Python-version-specific, i.e. use the distinction between / and //, but I'm not sure about that. To make this less ambiguous, we could possibly also start using from __future__ import division in our example scripts, so this would apply outside of the abstract code as well?

See also the discussion in #812 -- whatever we do, we have to do it carefully and raise warnings to make users aware of the changed semantics.

@thesamovar
Copy link
Member

Indeed this is quite problematic, and I agree there is no perfect solution. I think the options are:

  1. Use Python 2 semantics
  2. Use Python 3 semantics
  3. Use whichever semantics matches the users version of Python

I'm actually OK with any of these solutions. The advantage of Py2 semantics is that it also matches C semantics of always keeping things in the lowest possible type. Py3 semantics is the most unambiguous and is the most logical choice for the future. There's a good argument for Py2/3 semantics because it's somehow logical, but on the other hand I agree that it's weird that the same model would behave differently in the two versions.

Given all this I think I'd agree with a move towards Py3 semantics, but I wouldn't complicate our scripts by adding the future import.

mstimberg added a commit that referenced this issue Oct 25, 2017
This is not actually the case in the code (we set cdivision=True) and
should be properly handled for all targets (see #815).
mstimberg added a commit that referenced this issue Oct 25, 2017
This is not actually the case in the code (we set cdivision=True) and
should be properly handled for all targets (see #815).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants