compiler: Fix code generation for pow and fabs when using float32 #2504
Conversation
| assert ccode( | ||
| Abs(i1 - Constant('half', dtype=np.float64, default_value=0.5)) | ||
| ) == "fabs(i1 - half)" |
There was a problem hiding this comment.
I'm not happy with this, but don't 100% know how I'm supposed to generate a literal double precision 0.5 symbolically
There was a problem hiding this comment.
>>> ccode(Abs(i1 - np.float64(0.5)))
'fabs(i1 - 5.0e-1F)'It looks like the float64 is being demoted to float32, this could point to an issue elsewhere
| func = "abs" | ||
| elif self.single_prec(expr): | ||
| func = "fabsf" | ||
| elif any([isinstance(a, Real) for a in expr.args[0].args]): |
There was a problem hiding this comment.
From the look of it this seems un-necessary. You just need
If self.single_prec(expr) or self.single_prec():
fabsf
else:
# not integer or f32, default
fabs
There was a problem hiding this comment.
Not so, this is the example that I shared on slack, but will repeat here for reference:
>>> i1 = Dimension(name="i1")
>>> sympy_dtype(i1 - 0.5)
<class 'numpy.int32'>In the second part of the or in your example self.single_prec() will always return True, regardless of the types of the expression (unless the default is changed).
This code is a bit horrible, but ensures that the "default" is only used if there is a floating point number whose type cannot be determined. Otherwise strictly use the correct function call for float or double.
There was a problem hiding this comment.
Ah, but my test is wrong anyway... Okay the test is fine, the issue is trying to detect numpy float32 or float64 as sympy eagerly squashes them to a sympy.core.numbers.Float.
There was a problem hiding this comment.
I'm going to merge this, but perhaps in a subsequent PR you could attach a comment to that if
| assert ccode( | ||
| Abs(i1 - Constant('half', dtype=np.float64, default_value=0.5)) | ||
| ) == "fabs(i1 - half)" |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2504 +/- ##
==========================================
- Coverage 87.35% 87.29% -0.06%
==========================================
Files 238 238
Lines 45574 45645 +71
Branches 4035 4048 +13
==========================================
+ Hits 39813 39848 +35
- Misses 5079 5112 +33
- Partials 682 685 +3 ☔ View full report in Codecov by Sentry. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
I'm going to add the outstanding unresolved conversations to an issue and potentially tackle it in another PR, after some more thought/discussion. |
| elif any([isinstance(a, Real) for a in expr.args[0].args]): | ||
| # The previous condition isn't sufficient to detect case with | ||
| # Python `float`s in that case, fall back to the "default" | ||
| func = "fabsf" if self.single_prec() else "fabs" |
There was a problem hiding this comment.
I can see two self.single_prec tests. Isn;t this redundant here?
Can it be one check?
No description provided.