MR::Math::betaincreg(): Use lgamma_r()

Lestropie commented 6 months ago

Partially addresses #2857.

Hopefully the concern about lgamma_r() only being available on "some" systems should not be of concern for us given the target systems...

daljit46 commented 6 months ago

As mentioned in the meeting today, lgamma_r doesn't seem to be available on Windows and thus we'll need to account for that. I agree in principle with @jdtournier that this data race is benign in practice (btw I don't know for sure if lgamma actually uses the sign in its calculation). It's my opinion that we should not rely on undefined behaviour (even if in this case we may never invoke it because theoretically the compiler has rights to do whatever it wants if it detects UB) and make assumptions how the function would implemented on specific hardware (this has always turned out bad for me in the past).

Anyhow, we should at least use lgamma_r whenever it's available. @Lestropie For this purpose we can either rely on ifdefs like Eigen does or there is check_cxx_source_compiles in cmake, which can be used as follows:

include(CheckCXXSourceCompiles)
check_cxx_source_compiles("int main() { std::cout << lgamma_r(1.0) << std::endl; }" HAVE_LGAMMA_R)
# Then you can use HAVA_LGAMMA_R

Lestropie commented 6 months ago

@daljit46: I want to do something like 186356f715cae1fac306c7b1a8790b97adc267d1, where we both check for the presence of that function (making use of the extern "C" forward declaration), and if not available use a mutex lock to avoid undefined behaviour. However I couldn't find the cmake syntax to define a multi-line string. Also documentation suggests that check_cxx_source_compiles() stores the result, as opposed to defining vs. not defining the envvar. Are you able to give this the requisite cleanup?

Also, I'm revoking the "Closes #2857" in the OP. While this PR will hopefully resolve the issues around specifically std::lgamma(), as discussed in #2857 there's likely to still be some problems around the way I construct t->Z and F->Z lookup tables intended to be shared across threads, which will require further fixes.

Lestropie commented 6 months ago

Also, in core/math/betainc.h, it checks for MRTRIX_HAVE_EIGEN_UNSUPPORTED_SPECIAL_FUNCTIONS; but I don't see anywhere in cmake that is setting this environment variable. This used to be checked by the configure script. Was this perhaps lost in #2689?