Uh oh!
There was an error while loading. Please reload this page.
Add gamma function to f32 and f64 - #99747
Conversation
rust-highfive
commented
Jul 26, 2022
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon. Please see the contribution instructions for more information. |
rustbot
commented
Jul 26, 2022
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
joshtriplett
commented
Jul 28, 2022
LGTM. Please file a tracking issue, then r=me. |
ankane
commented
Jul 28, 2022
Thanks @joshtriplett, tracking issue added. |
joshtriplett
commented
Jul 28, 2022
@bors r+ rollup |
bors
commented
Jul 28, 2022
Add gamma function to f32 and f64 Adds the [gamma function](https://en.wikipedia.org/wiki/Gamma_function) to `f32` and `f64` (`tgamma` and `tgammaf` from C). Refs: - rust-lang/rfcs#864 - rust-lang#18271
Dylan-DPC
commented
Jul 28, 2022
failed in rollup |
Dylan-DPC
commented
Jul 28, 2022
@bors r- |
| #[test] | ||
| fn test_gamma() { | ||
| assert_eq!(0.0f32.gamma(), f32::INFINITY); |
There was a problem hiding this comment.
This test causes compile time errors on platforms like x86_64-fortanix-unknown-sgx and probably others:
12:04:56 = note: rust-lld: error: undefined symbol: tgammaf
12:04:56 >>> referenced by std.143fe29c-cgu.2
12:04:56 >>> /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-e08c880d8c834560.std.143fe29c-cgu.2.rcgu.o:(std::f32::tests::test_gamma::he9b80e8a0cc72b6a)
12:04:56 >>> referenced by std.143fe29c-cgu.2
12:04:56 >>> /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-e08c880d8c834560.std.143fe29c-cgu.2.rcgu.o:(std::f32::tests::test_gamma::he9b80e8a0cc72b6a)
12:04:56 >>> referenced by std.143fe29c-cgu.2
12:04:56 >>> /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-e08c880d8c834560.std.143fe29c-cgu.2.rcgu.o:(std::f32::tests::test_gamma::he9b80e8a0cc72b6a)
12:04:56 >>> referenced 6 more times
12:04:56 12:04:56 rust-lld: error: undefined symbol: tgamma
12:04:56 >>> referenced by std.143fe29c-cgu.3
12:04:56 >>> /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-e08c880d8c834560.std.143fe29c-cgu.3.rcgu.o:(std::f64::tests::test_gamma::hff115a05914e5bdc)
12:04:56 >>> referenced by std.143fe29c-cgu.3
12:04:56 >>> /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-e08c880d8c834560.std.143fe29c-cgu.3.rcgu.o:(std::f64::tests::test_gamma::hff115a05914e5bdc)
12:04:56 >>> referenced by std.143fe29c-cgu.3
12:04:56 >>> /home/jenkins/workspace/rust-sgx-ci/rust/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-e08c880d8c834560.std.143fe29c-cgu.3.rcgu.o:(std::f64::tests::test_gamma::hff115a05914e5bdc)
12:04:56 >>> referenced 6 more times
12:04:56 12:04:56 12:04:56 error: could not compile `std` due to previous error
Looking into it, these tgamma and tgammaf symbols exists, but are not exported from the compiler-builtins crate. These should probably be added here.
There was a problem hiding this comment.
Thanks @raoulstrackx! Submitted a PR there: rust-lang/compiler-builtins#482
| #[test] | ||
| fn test_gamma() { | ||
| assert_eq!(0.0f64.gamma(), f64::INFINITY); |
There was a problem hiding this comment.
Causes similar issues as above
mati865
commented
Jul 29, 2022
I think this PR should never be rolled up because it's going to fail on more targets like @bors rollup=never |
bors
commented
Jul 29, 2022
@mati865: 🔑 Insufficient privileges: not in try users |
Dylan-DPC
commented
Jul 29, 2022
@bors rollup=never |
joshtriplett
commented
Jul 29, 2022
@mati865 I was expecting that these symbols were being provided by compiler-builtins fallbacks on all platforms. We do need fallbacks for platforms that don't have these. |
mati865
commented
Jul 29, 2022
@joshtriplett I'm away until next week but mingw-w64 seems to provide own implementation when using MSVCRT but it returns wrong results for certain values. This issue has been fixed 3 years ago but Rust uses 5 years old release. |
ankane
commented
Jul 30, 2022
Just bumped |
bors
commented
Aug 13, 2022
☔ The latest upstream changes (presumably #100380) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
commented
Jul 31, 2023
⌛ Testing commit 793e952d9beb8ed40ecbfa188e37882dfdb15555 with merge ee51e45123e931937c445a123b67d24d3e3cd4fa... |
This comment has been minimized.
This comment has been minimized.
bors
commented
Jul 31, 2023
💔 Test failed - checks-actions |
workingjubilee
commented
Jul 31, 2023
r? @workingjubilee |
ankane
commented
Aug 7, 2023
Bumped @rustbot ready |
workingjubilee
commented
Aug 8, 2023
I hope we can solve the underlying problem there, soon. ( And that it doesn't crop up again for this PR... ) @bors r+ |
bors
commented
Aug 8, 2023
bors
commented
Aug 9, 2023
bors
commented
Aug 9, 2023
☀️ Test successful - checks-actions |
ankane
commented
Aug 9, 2023
Thanks everyone for helping with this! |
rust-timer
commented
Aug 9, 2023
Finished benchmarking commit (8838c73): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 633.338s -> 632.915s (-0.07%) |
| unsafe { cmath::tgammaf(self) } | ||
| } | ||
| /// Returns the natural logarithm of the gamma function. |
There was a problem hiding this comment.
This is wrong, it returns the natural log of the absolute value of the gamma function.
There was a problem hiding this comment.
#[test]fntest_ln_gamma(){assert_approx_eq!(1.0f64.ln_gamma().0,0.0f64);assert_eq!(1.0f64.ln_gamma().1,1);assert_approx_eq!(2.0f64.ln_gamma().0,0.0f64);assert_eq!(2.0f64.ln_gamma().1,1);assert_approx_eq!(3.0f64.ln_gamma().0,2.0f64.ln());assert_eq!(3.0f64.ln_gamma().1,1);assert_approx_eq!((-0.5f64).ln_gamma().0,(2.0* consts::PI.sqrt()).ln());assert_eq!((-0.5f64).ln_gamma().1, -1);}? am I just missing something, if so why does the final test case pass...
There was a problem hiding this comment.
Γ(-0.5f64) is negative, its natural log should be undefined
There was a problem hiding this comment.
...well, it sounds obvious now that you've said it, so...!
#114754
Adds the gamma function to
f32andf64(tgammaandtgammaffrom C).Refs: