nest / nestml

A domain specific language for neuron and synapse models in spiking neural network simulation
GNU General Public License v2.0
48 stars 45 forks source link

Divisions of integers not converted to divisions of doubles in generated C++ code of nestml functions #902

Open WillemWybo opened 1 year ago

WillemWybo commented 1 year ago

I encountered an issue with expressions in nestml functions. I had expressions which featured a division of integers, i.e. 4 / 7, which in the C++ code were not converted to divisions of doubles, i.e. 4./7.. I feel that this mixes abstraction levels; when writing models in a high-level language, I would expect more pythonic behaviour, and 4 / 7 should return 0.571429 and not 0.

Furthermore, I suspect the issue would be fixed by simply calling sympy.printing.ccode(sympy.sympify(expr)) with expr the expression in the function block, unless there are some subtle aspects of the code generation for nestml functions that I am missing.

clinssen commented 1 year ago

@WillemWybo : thanks for reporting this. I will look into it, could you help in define the semantics of the division operator? Should we be Python-like and have / always represent floating point division, only (possibly) coercing the result to integer depending on the type of the variable we assign it to? I.e. (see especially x9):

parameters:
    x1 real = 4 / 7                   # expect 0.57..
    x2 real = 4
    x3 real = 7
    x4 real = x2 / x3                 # expect 0.57..
    x5 integer = 4
    x6 integer = 7
    x7 integer = x5 / x6              # expect 0
    x8 real = x5 / x6                 # expect 0.57..
    x9 real = x5 / x6 + x5 / x6       # expect 1 (C semantics would yield 0)
    x10 integer = x2 / x3 + x2 / x3   # expect 1
clinssen commented 1 year ago

We could also introduce the Python // operator for integer (actually, "floor") division.

This means that a/b would always be rendered in C++ as something like ((float)a)/((float)b).

But note that in Python, -3//2 equals -2 ("floor division"), whereas in C -3/2 equals -1 ("true" integer division). So we should be diligent about defining the semantics.

WillemWybo commented 1 year ago

@clinssen you raise an interesting point. My functions were returning real. It would make sense to make the division behaviour dependent upon the expected return type, as you propose.

Sympy seems to take the explicit approach, where sp.printing.ccode() returns floor(a/b) for a//b and (float)a/(float)b for a/b. I guess if the variable is assigned to an integer it would be reasonable to expect the behaviour (int)floor(a/b) or (int)((float)a/(float)b), the latter case being the true integer division.

Note that if the variable is part of some complex expression, i.e. exp(a/b + c), and then converted to an integer, I would still expect the behaviour (int)exp((float)a / (float)b + (float)c), but maybe this is personal taste.

clinssen commented 1 year ago

We would suggest to make / always be Python3-like floating point division, and to make // always be the C-like "true" integer division, where the result is always of integer type.

WillemWybo commented 1 year ago

I would personally rather follow the Python3 convention for // as well, but no strong opinion either way