Handle sympy integers #1202
Handle sympy integers #1202
Conversation
The optimization step can rewrite e.g. "x + 1" as "x + 1.0" if x is a float
| __k_4_v = -dt*(__k_3_v + v)/tau | ||
| _v = 0.166666666666667*__k_1_v + 0.333333333333333*__k_2_v + 0.333333333333333*__k_3_v + 0.166666666666667*__k_4_v + v | ||
| _v = __k_1_v/6 + __k_2_v/3 + __k_3_v/3 + __k_4_v/6 + v |
There was a problem hiding this comment.
This I'm not sure about. If we know that x is a float, then we can safely convert x/2 to 0.5*x and this will be faster. Probably the compiler does it already if -funsafe-math-optimizations is on, but would be interesting to know if there are cases where the new version would cause it to run slower. What do you think?
There was a problem hiding this comment.
This is not really about optimized code here: when we define the RK4 state updater, we write things like k_3 = dt*f(x+k_2/2,t+dt/2) – before, sympy converted this to floating point numbers and to multiplications along the way. I don't think this is for sympy to do where we are still at the level of mathematics. Optimizing the generated code is a different question, I don't think we currently turn divisions into multiplications but we could (even though I doubt it makes a measurable difference). We can run our benchmark suite after merging, but I'm not really worried about this. I'm more worried that results change slightly due to the numerical difference, this might be worth investigating further.
There was a problem hiding this comment.
Right, I agree that sympy shouldn't be doing that for us because it doesn't know that it might be relevant that it's an integer.
| @@ -33,7 +33,7 @@ def get_conditionally_linear_system(eqs, variables=None): | |||
| -------- | |||
| >>> from brian2 import Equations | |||
| >>> eqs = Equations(""" | |||
| ... dv/dt = (-v + w**2) / tau : 1 | |||
| ... dv/dt = (-v + w**2.0) / tau : 1 | |||
There was a problem hiding this comment.
Does this have performance implications?
There was a problem hiding this comment.
Given that it is just a change in the doctest, I'm quite sure it does not ;)
But in general it is the same issue as above: before w**2 and w**2.0 where both converted to w**2.0 when we handed them to sympy – now they are given to sympy as either an integer or a float.
| ('x = 1.0/x', 'x = 1.0/x'), | ||
| ('x = 1.0', 'x = 1.0'), | ||
| ('x = 2.0*(x + 1.0)', 'x = 2.0*(x + 1.0)'), | ||
| ('x = clip(x + y, 0.0, inf)', 'x = clip(x + y, 0.0, inf)'), |
There was a problem hiding this comment.
Wait, I'm confused: why were these integers before and now floats?
There was a problem hiding this comment.
Yea, this is confusing... The explication is: we are using sympy behind the scenes in this test to compare the optimized expressions. This is to avoid that x + 1 is considered as different from 1 + x, for example.
Here we are testing the codegen optimization and during this process we do sometimes replace integers by floats as a side-effect (e.g. x + 1 will turn into x + 1.0 if x is a float). This is fine and nothing to worry about, but it made the test fail due to sympy making a difference between the two expressions now. When we are testing that x = 1/x does not change during optimization, we are not caring about whether the 1 continues to be an integer – instead of making the test comparison more complicated, I simply turned all values into floats...
There was a problem hiding this comment.
Ahhh I see. OK, in that case I'm all good! Happy to merge.
As noted in #812 , we always converted integers to float whenever we use sympy (most importantly for differential equations). Back in the days we could not fix it because it led to problems with floor division. This has been fixed in sympy quite a while ago, in sympy 1.1 (we already require sympy >=1.2), so this is no longer an issue. With this PR, we take care to represent integers as integers in sympy, which fixes #1199 .
Note that there are still places where we convert integers to floats that are unrelated to sympy. For an expression
x + 1, wherexis a floating point variable (and therefore the whole expression is floating point), we try to rearrange things during the optimization step (e.g. to replacex + 0byx), which leads to the expression being converted tox + 1.0. Changing this would be quite some work, but I don't think it is necessary.