To fix inconsistency of digamma with SciPy

[iramazanli]

Fixes {#49015}

[facebook-github-bot]

💊 CI failures summary and remediations

As of commit 30164ee (more details on the Dr. CI page):

---

• 1/1 failures possibly* introduced in this PR
  • 1/1 non-scanned failure(s)

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[mruberry]

The ROCm build failure is real:

Apr 22 21:52:09 lld: error: undefined symbol: modf(float, float*)
Apr 22 21:52:09 >>> referenced by /tmp/UnaryGammaKernels-gfx900-baa730.o:(_ZN2at6native6legacy18elementwise_kernelILi512ELi1EZNS0_15gpu_kernel_implIZZZNS0_19digamma_kernel_cudaERNS_18TensorIteratorBaseEENKUlvE_clEvENKUlvE2_clEvEUlfE_EEvS5_RKT_EUliE_EEviT1_)
Apr 22 21:52:09 >>> referenced by /tmp/UnaryGammaKernels-gfx900-baa730.o:(_ZN2at6native6legacy18elementwise_kernelILi512ELi1EZNS0_15gpu_kernel_implIZZZNS0_19digamma_kernel_cudaERNS_18TensorIteratorBaseEENKUlvE_clEvENKUlvE2_clEvEUlfE_EEvS5_RKT_EUliE_EEviT1_)
Apr 22 21:52:09 >>> referenced by /tmp/UnaryGammaKernels-gfx900-baa730.o:(void at::native::modern::elementwise_kernel<at::native::digamma_kernel_cuda(at::TensorIteratorBase&)::'lambda'()::operator()() const::'lambda2'()::operator()() const::'lambda'(float), at::detail::Array<char*, 2> >(int, at::native::digamma_kernel_cuda(at::TensorIteratorBase&)::'lambda'()::operator()() const::'lambda2'()::operator()() const::'lambda'(float), at::detail::Array<char*, 2>))
Apr 22 21:52:09 >>> referenced 53 more times
Apr 22 21:52:09 clang-12: error: amdgcn-link command failed with exit code 1 (use -v to see invocation)
Apr 22 21:52:09 CMake Error at torch_hip_generated_UnaryGammaKernels.hip.o.cmake:192 (message):
Apr 22 21:52:09   Error generating file
Apr 22 21:52:09   /var/lib/jenkins/workspace/build/caffe2/CMakeFiles/torch_hip.dir/__/aten/src/ATen/native/hip/./torch_hip_generated_UnaryGammaKernels.hip.o
Apr 22 21:52:09 
Apr 22 21:52:09 
Apr 22 21:52:09 caffe2/CMakeFiles/torch_hip.dir/build.make:1307: recipe for target 'caffe2/CMakeFiles/torch_hip.dir/__/aten/src/ATen/native/hip/torch_hip_generated_UnaryGammaKernels.hip.o' failed
Apr 22 21:52:09 make[2]: *** [caffe2/CMakeFiles/torch_hip.dir/__/aten/src/ATen/native/hip/torch_hip_generated_UnaryGammaKernels.hip.o] Error 1

Maybe ::modf or std::modf would appease the ROCm build?

[iramazanli]

hi @jeffdaily, we are trying to use the 'modf' function, to compute the digamma function more accurately. We have tried both ::modf and std::modf in the cuda implementation : aten/src/ATen/native/cuda/Math.cuh in this PR but unfortunatly ROCm seem doesn't support neither of these cases. Is there an alternative here we can use ?

[jeffdaily]

The device functions I'm finding are the ones that follow the C++11 naming convention, rather than overloading modf.

• float modff(float x, float* iptr)
• double modf(double x, double* iptr)

Might that be the reason for the error: undefined symbol: modf(float, float*)?

CC @sunway513 @jithunnair-amd

[mruberry]

"fractiontional" -> "fractional"
"bir errors" -> "big errors"

But this comment can also be simplified a little:

Extracts the fractional part of x as r, since tan(pi * r) is more numerically accurate than tan(pi * x). While these operations are mathematically equivalent since both x and r are in radians and tan() has a periodicity of pi, in practice the computation of pi * x is a source of error (when |x| > 1). 

[iramazanli]

this is indeed very nice choice of explanation what happens in the following code. Thanks for the suggestion.

[mruberry]

Since this is the issue this PR is fixing, this note should be removed, too

[iramazanli]

done :)

[mruberry]

The function and test update look correct now, there are just a couple tweaks needed for the comments

[facebook-github-bot]

@iramazanli has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[codecov]

Codecov Report

Merging #56689 (30164ee) into master (e97c17a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #56689      +/-   ##
==========================================
- Coverage   77.20%   77.20%   -0.01%     
==========================================
  Files        1948     1948              
  Lines      193573   193575       +2     
==========================================
- Hits       149455   149452       -3     
- Misses      44118    44123       +5     

[mruberry]

Cool; nice to have this fixed!

[facebook-github-bot]

@iramazanli merged this pull request in 6e826ca.