compiler: Check sympy_type returns a floating point type in the code printer
#2515
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2515 +/- ##
===========================================
- Coverage 87.28% 57.55% -29.73%
===========================================
Files 238 238
Lines 45703 45734 +31
Branches 4057 4058 +1
===========================================
- Hits 39892 26324 -13568
- Misses 5126 18583 +13457
- Partials 685 827 +142 ☔ View full report in Codecov by Sentry. |
| @@ -313,7 +313,7 @@ def test_cos_vs_cosf(): | |||
|
|
|||
| # Doesn't make much sense, but it's legal | |||
| c = dSymbol('c', dtype=np.int32) | |||
| assert ccode(cos(c)) == "cos(c)" | |||
| assert ccode(cos(c)) == "cosf(c)" | |||
There was a problem hiding this comment.
so ccode(Abs(c)) will be absf(c) which is incorrect it should be abs(c).
There was a problem hiding this comment.
You are correct, there is a bug here with your specific example. However, I think I introduced it in my last PR and it is specific to abs/fabsf/fabs, which I will address.
But, in this test, why should the resultant code be cos(c) over cosf(c) when c is an integer type?
There was a problem hiding this comment.
Well cos is technically not defined for integers so it will convert to float or double depending on the function used so I guess the test is broken in itself and should use a function that is valid for integers
| dtype = sympy_dtype(expr) if expr is not None else self.dtype | ||
| # Check that the dtype is a floating point type | ||
| dtype = dtype if np.issubdtype(dtype, np.floating) else self.dtype |
There was a problem hiding this comment.
I don't think this is valid, this will return "is float" for integer and will break a lot of cases. This is probably only necessary for some limited cases like cos but not in general
There was a problem hiding this comment.
I disagree a little bit here, the method is called single_prec not is_float. If an expression only containing integers is passed, it is unclear that the result should return anything other than the default, as determined here by self.dtype.
I could see an argument for following the sympy behaviour of returning None if the precision cannot be determined, but this won't change much as bool(None) == bool(False) == False.
In the bigger picture, there are a lot of issues coinciding here, it seems that sympy_dtype isn't quite fit for purpose and the printer is doing some rather rough type inference. Personally, I think home-grown type inference should be avoided here (at least for the purposes of this PR), but I am open to further suggestions.
There was a problem hiding this comment.
the method is called single_prec not is_float
Well, sure, that's the name we used when we made it, but this is effectively an is_float. This merely checks if it needs the float literal.
If an expression only containing integers is passed it is unclear that the result should return anything other than the default, as determined here by self.dtype
self.dtype is only a last resort default for expression that cannot have a type determined, the type of the expression if determined is always the one that should be used irrespective of what that type is.
This aims to fix a bug in code generation for certain platforms