Fixed dace::math::pow for Integer case
#1748
Conversation
The new specialization works for integers. So if all arguments are integer the return value is guaranteed to be integer as well. This should also solve the issue with the fft test mentioned by Tal. https://github.com/spcl/dace/pull/1742/files#diff-a81c4594b19da98f6516283a50f6958b04976ae2913f097358141b2681868f3eR158
However, to work we need spcl#1747.
philip-paul-mueller
changed the title
dace::math::pow for Integer case
|
@philip-paul-mueller That's a lot of modern C++. Can you verify that it conforms to C++14? |
|
Yes everything is C++14, actually it looks a bit strange because for example I spotted a small error in the current implementation and also one in |
|
Actually If I have time I want to go through the runtime and fix some things there (for example the |
The original signature of `ipow` only accepted an unsigned integer as exponent. However, it seems that it was called using integers as exponents. Thus there is now a specialization for this case. For the sake of argument let's consider the following situation `ipow(a, b)`: - If `b` was zero then the previous implementation would return `a`, the new implementation returns 1. - If `b` was negative then it would perform a lot of iterations, because a signed integer was converted to an unsigned one. In the new implementation, the function would return 0, which is in agreement with what `pow` does.
I now combined both versions into one, this makes it similar to what `pow` is. However another version would be to put the enable if on the arguments, but I thought it would be too ugly.
We cannot do so yet, definitely not before 1.0. As far as I understand some of our target platforms do not support it and it will change our compiler requirements. |
|
I currently have a strange error with template instantiation. |
After some digging I found out that `ipow` is only called for positive integers that are known at compile time. And I know now whym because the `half` vectors do not play nice.
| template< | ||
| typename T1, | ||
| typename T2, | ||
| typename = std::enable_if_t<std::is_integral<T1>::value && std::is_integral<T2>::value> |
There was a problem hiding this comment.
So, if both a and b are integers (signed or unsigned), this path will be followed? In other words, if we want to compute, e.g., 3^-5, we need to convert one of them to float. Are we sure this is the behavior we want?
There was a problem hiding this comment.
This is correct.
But we kind of need this behaviour, to enable the FFT test this commit enables.
There the problem is that a size of an array is given as a = np.ones(2**3).
Which the code generator translates to
double* a;
a = new double[pow(2, 3)];If pow would return a double, then it is a compilation error.
There are technically three things we can do:
- Leave it as it is.
- Adding the specialization that if the exponent is an unsigned int then we return a double and hope that expressions such as above are never called where the exponent is unsigned, i.e.
unsigned int b = 4;
//...
double* a;
a = new double[pow(2, b)];- Patch the code generator that sizes in
new[]expressions are always casted tostd::size_t.
However, this would be another PR.
What is your opinion on this @tbennun ?
There was a problem hiding this comment.
I agree with @alexnick83. The reason we didn't have these pow implementations in the first place was because of negative exponents. I think we should use ipow when we know something or the equivalent version only in the case of an unsigned exponent and integer base.
There was a problem hiding this comment.
I looked it up in the code generator, ipow is only used if the exponent is known at code generation time.
This is not the case in the FFT test, there the exponent is not known and thus pow is selected.
The reason why it lead to the compilation error in the FFT test was because the arguments of pow where int64 and since there is only an overload for int and unsigned int, the template was selected.
However, even before the expression 5^-3 would have resulted in the value 0, given that both were integer.
For the reasons outlined above, I do not think that this PR changes anything big.
But how should I proceed @alexnick83 @tbennun
PS: I also tried to implement ipow for negative exponents and that failed with some compilation bug related to half, which I did not understood.
Added a pow specialization for
dace::math::pow.The new specialization works for integers if all arguments are integer the return value is guaranteed to be integer as well.
This should also solve the issue with the fft test mentioned by Tal (https://github.com/spcl/dace/pull/1742/files#diff-a81c4594b19da98f6516283a50f6958b04976ae2913f097358141b2681868f3eR158).
However, to work we need #1747.