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

[Lestropie]

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]

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]

@daljit46: I want to do something like 186356f, 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]

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?

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[Lestropie]

Can confirm same issue on my local Windows machine as what's happening on CI: the check_cxx_source_compiles() test succeeds, and MRTRIX_HAVE_LGAMMA_R takes value 1; compilation then fails.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[daljit46]

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?

Yes, I think that's right. We can re-introduce this again I suppose (e.g. using target_compile_definitions), but is this still necessary? I mean that with the CMake transition we are effectively targeting Ubuntu 20.04 as our minimum platform. So, support for special functions in Eigen should be available on all distros we are targeting.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"