Replace uses of long with more portable types
#53
Conversation
`long` is unportable, since its width depends a lot on the target platform: it’s 32-bit on 32 bit platforms and Windows, but 64-bit elsewhere. `long k` in `lambertw.h` is replaced with `int64_t`, since it can be any integer and is castable to a float. `long n` in the spherical Bessel functions is replaced with `uint64_t`. This change also fixes three bugs: + Previously, `int`s were used in loops up to `n`, but this is the wrong type. This is changed to use `uint64_t`. + `n + 1` was used liberally before, but this causes UB when `n` is its maximum value for the type. We instead cast to the float type before adding one, thus avoiding UB (or in the case of the new unsigned type, overflow). + `std::pow(-1, n)` was previously used – however, this is incorrect when `n` is a `long`, since it will be implicitly converted to an `int`. We replace these with a simple ternary to control the sign.
SabrinaJewson
force-pushed
the
long-long-man
branch
from
6719365 to
a1c17b0
Compare
|
So there were actually reasons we went with With regards to |
|
For context, I’m trying to use xsf for embedding in a different language, so I’m just trying to make things as easy as possible for this purpose.
I am sympathic to this, but again it makes it a lot more difficult to use from other languages who would expect an unsigned type to be used here – that is, I favour the unsigned version because it provides strictly more functionality than the signed version. I suppose it comes down to whether the vision for this library is to provide a universally applicable suite of special functions – in which case Edit: Maybe Scipy in particular can try adopting |
|
Thanks @SabrinaJewson. I think @izaid just wants to be cautious about changing things without being aware of their downstream impact in SciPy etc. For these particular cases, I don't think the use of It's good you pointed this out. The use of See below that the wrapper converts the array # TODO: special expert should inspect this
# interception; better place to do it?
k = np.asarray(k, dtype=np.dtype("long"))
return _lambertw(z, k, tol)
For functions in This stuff came up recently in SciPy here, scipy/scipy#22795 (comment). I don't think we can just always use template<typename T>
constexpr bool is_supported_int_v =
std::is_same_v<T, int32_t> || std::is_same_v<T, int64_t>;
template<typename T>
using enable_if_supported_int_t = std::enable_if_t<is_supported_int_v<T>, int>;and then template <typename T, enable_if_supported_int_t<T> = 0>
XSF_HOST_DEVICE inline std::complex<float> lambertw(std::complex<float> z, T k, float tol) {As for signed vs unsigned integers. Can you elaborate on the problems you're running into wrapping C++ functions which take signed integers? If it was consistently documented that public functions in |
|
scipy/scipy#19462 is still open |
|
Thanks for the long response! That seems very sensible.
In the language I’m trying to wrap in (Rust), xsf using signed types mean I have three options:
The idiomatic thing to do is just to support the full range of the unsigned type, which is why I have a preference for it.
I suppose I was under the impression that Scipy in general already did this with
Right – and that’s where the friction between C/C++ and other languages comes in. This guideline exists because of the syntactic limitations of C/C++, but from my perspective it’s odd that this should leak into public APIs. In my opinion, By the way, I’m perfectly happy for array lengths to be signed values, since that doesn’t actually affect the range (arrays can’t be that large anyway). So this is just about the integers that are accepted for the purpose of casting to floats. |
|
I see. Looking into it more. In the case of Spherical Bessel functions, restriction to nonnegative integers is due to limitations in the implementation, not due to inherent properties of Spherical Bessel functions. The order in principle can be a general complex number. This sort of thing comes up a lot, where there are domain restrictions that are due to limitations in an implementation. The convention in this case has been to return Since restriction to nonnegative ints is an implementation detail, I'd be hesitant to change the input type to an unsigned type just to match the domain limitation. If you insist on unsigned integers in your wrappers, of the options you gave above for dealing with unsigned integers which exceed the largest signed integer of the same width, 3) would be the most faithful to the conventions used in |
Oh, that’s a good point. In that case, yes I agree it should be signed. What about cases like the binomial distribution mentioned above? |
Additionally, add checks in `sph_bessel_*_jac` functions to prevent overflow when calculating `n - 1`.
|
I updated the PR to use the |
|
Huh. macOS CI is failing and I don’t know why… unfortunately I don’t have a Mac. CUDA is also failing, and I have absolutely zero experience with that… 😅 Any ideas from people who know C++ better than me? it just says this: macOS build error |
There are analytic continuations there too. I remember this coming up here, scipy/scipy#2414 (comment). One complication is that when treating a function as CDF function of a distribution with compact support, evaluation at points in the domain greater than the support yields result 1, and evaluation at points less than the support gives output 0. But if you were to do extend the domain through analytic continuation, you get a different extension. There's a tension between extension through analytic continuation and extension by general statistical convention. To allow flexibility to libraries that want to wrap
The CuPy tests are failing because the version of CuPy in CI is still stuck on C++11, but the changes require C++17. I bumped the C++ language standard for special functions in CuPy to C++17 in cupy/cupy#9159, but this hasn't made it into a CuPy release yet. I'll think of something. I'm not sure what's happening on macOS, but I have a Mac mini I can test this out on. |
I believe you can pass in an argument to the CuPy ElementwiseKernel that enables C++17. |
Exactly. The tests currently use |
longis unportable, since its width depends a lot on the target platform: it’s 32-bit on 32 bit platforms and Windows, but 64-bit elsewhere.long kinlambertw.his replaced withint64_t, since it can be any integer and is castable to a float.long nin the spherical Bessel functions is replaced withuint64_t. This change also fixes three bugs:ints were used in loops up ton, but this is the wrong type. This is changed to useuint64_t.n + 1was used liberally before, but this causes UB whennis its maximum value for the type. We instead cast to the float type before adding one, thus avoiding UB (or in the case of the new unsigned type, overflow).std::pow(-1, n)was previously used – however, this is incorrect whennis along, since it will be implicitly converted to anint. We replace these with a simple ternary to control the sign.