Fix asinh of negative values #72486

Ralith · 2020-05-23T04:06:36Z

Rust's current implementation of asinh has large errors in its negative range. ~~These are (mostly) not numerical, but rather seem due to an incorrect implementation.~~ This appears to be due to avoidable catastrophic cancellation.
Playground before/after.
glibc uses abs here.

Many thanks to @danieldeankon for finding this weird behavior, @jebrosen for diagnosing it, and @toasteater for identifying the probable implementation error!

rust-highfive · 2020-05-23T04:06:39Z

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

ghost · 2020-05-23T05:15:20Z

To think about it after the fact, this probably is a numerical issue, though. self ((self * self) 1.0).sqrt() is supposed to produce a very small value whose .ln() is equal to -(self.abs() ((self * self) 1.0).sqrt()).ln(). In practice, however, once the absolute value of self goes above 2^26 (67108864, compare to 67452095 in the original example), self * self will be around 2^53, and the 1.0 will no longer work. That makes self ((self * self) 1.0).sqrt() dangerously close (or equal to) zero, leading to the precision problems and -infs in the original example.

The .abs() addition is a transformation that works because asinh is odd, and additions are much more permissive when there are no negative numbers involved.

shepmaster · 2020-05-23T10:56:41Z

I don't have the numerical chops to evaluate this, so I'll be reassigning, but it seems like a really bad idea to "fix" this without (a) even stating what the problem is in the commit message (links to external sites are great, but ultimately very fragile) (b) adding any kind of automated test to prevent regressions.

r? @cramertj

Ralith · 2020-05-23T21:40:49Z

Good points. Added discussion of the numerical issue and rationale for the correctness of the solution to the commit, and threw in a basic regression test.

src/libstd/f64.rs

src/libstd/f32.rs

When `x` has large magnitude, `x ((x * x) 1.0).sqrt()` approaches `x x.abs()`. For negative values of `x`, this leads to catastrophic cancellation, resulting in large errors or even 0 being passed to `ln`, producing incorrect results including `-inf`. Becuase asinh is an odd function, i.e. -asinh(x) = asinh(-x) for all x, we can avoid the catastrophic cancellation and obtain correct results by taking the absolute value of `self` for the first term. `self * self` is always positive, so in effect this gives us `x.abs().asinh().copysign(x)` which as discussed above is algebraically equivalent, but is much more accurate.

Ralith · 2020-06-18T23:35:05Z

Ping?

Mark-Simulacrum · 2020-06-19T00:35:42Z

Hm, I'm not sure who a good reviewer here would be.

Cc @rust-lang/libs - do we have anyone?

dtolnay

I can review this kind of thing.

Asinh is odd, and it's clear we're getting better results in the positive range, so I'm happy to take this improvement. libclc does the abs there too.

// Arguments such that 4.0 <= abs(x) <= 1/sqrt(epsilon) are
// approximated by asinhf(x) = ln(abs(x)   sqrt(x*x 1))
// with the sign of x (see Abramowitz and Stegun 4.6.20)

Following up on this, it would be good to look at an appropriately licensed high quality implementation and consider whether we should pick up what they do on some of the other intervals, namely close to zero and very far from zero where ln(abs(x) sqrt(x*x 1)) still isn't good.

dtolnay · 2020-06-19T01:19:18Z

@bors r

bors · 2020-06-19T01:19:19Z

📌 Commit 730f736 has been approved by dtolnay

src/libstd/f32.rs

dtolnay · 2020-06-19T01:59:31Z

@bors r

bors · 2020-06-19T01:59:32Z

📌 Commit 35a2915 has been approved by dtolnay

RalfJung · 2020-06-19T06:54:08Z

@bors rollup

@ghost

Rollup of 13 pull requests Successful merges: - rust-lang#70740 (Enabling static-pie for musl) - rust-lang#72331 (Report error when casting an C-like enum implementing Drop) - rust-lang#72486 (Fix asinh of negative values) - rust-lang#72497 (tag/niche terminology cleanup) - rust-lang#72999 (Create self-contained directory and move there some of external binaries/libs) - rust-lang#73130 (Remove const prop for indirects) - rust-lang#73142 (Ensure std benchmarks get tested.) - rust-lang#73305 (Disallow loading crates with non-ascii identifier name.) - rust-lang#73346 (Add rust specific features to print target features) - rust-lang#73362 (Test that bounds checks are elided when slice len is checked up-front) - rust-lang#73459 (Reduce pointer casts in Box::into_boxed_slice) - rust-lang#73464 (Document format correction) - rust-lang#73479 (Minor tweaks to liballoc) Failed merges: r? @ghost

according to various people on tech-pkg@, there are no problems with the Firefox build Version 1.46.0 (2020-08-27) ========================== Language -------- - [`if`, `match`, and `loop` expressions can now be used in const functions.][72437] - [Additionally you are now also able to coerce and cast to slices (`&[T]`) in const functions.][73862] - [The `#[track_caller]` attribute can now be added to functions to use the function's caller's location information for panic messages.][72445] - [Recursively indexing into tuples no longer needs parentheses.][71322] E.g. `x.0.0` over `(x.0).0`. - [`mem::transmute` can now be used in static and constants.][72920] **Note** You currently can't use `mem::transmute` in constant functions. Compiler -------- - [You can now use the `cdylib` target on Apple iOS and tvOS platforms.][73516] - [Enabled static "Position Independent Executables" by default for `x86_64-unknown-linux-musl`.][70740] Libraries --------- - [`mem::forget` is now a `const fn`.][73887] - [`String` now implements `From<char>`.][73466] - [The `leading_ones`, and `trailing_ones` methods have been stabilised for all integer types.][73032] - [`vec::IntoIter<T>` now implements `AsRef<[T]>`.][72583] - [All non-zero integer types (`NonZeroU8`) now implement `TryFrom` for their zero-able equivalent (e.g. `TryFrom<u8>`).][72717] - [`&[T]` and `&mut [T]` now implement `PartialEq<Vec<T>>`.][71660] - [`(String, u16)` now implements `ToSocketAddrs`.][73007] - [`vec::Drain<'_, T>` now implements `AsRef<[T]>`.][72584] Stabilized APIs --------------- - [`Option::zip`] - [`vec::Drain::as_slice`] Cargo ----- Added a number of new environment variables that are now available when compiling your crate. - [`CARGO_BIN_NAME` and `CARGO_CRATE_NAME`][cargo/8270] Providing the name of the specific binary being compiled and the name of the crate. - [`CARGO_PKG_LICENSE`][cargo/8325] The license from the manifest of the package. - [`CARGO_PKG_LICENSE_FILE`][cargo/8387] The path to the license file. Compatibility Notes ------------------- - [The target configuration option `abi_blacklist` has been renamed to `unsupported_abis`.][74150] The old name will still continue to work. - [Rustc will now warn if you cast a C-like enum that implements `Drop`.][72331] This was previously accepted but will become a hard error in a future release. - [Rustc will fail to compile if you have a struct with `#[repr(i128)]` or `#[repr(u128)]`.][74109] This representation is currently only allowed on `enum`s. - [Tokens passed to `macro_rules!` are now always captured.][73293] This helps ensure that spans have the correct information, and may cause breakage if you were relying on receiving spans with dummy information. - [The InnoSetup installer for Windows is no longer available.][72569] This was a legacy installer that was replaced by a MSI installer a few years ago but was still being built. - [`{f32, f64}::asinh` now returns the correct values for negative numbers.][72486] - [Rustc will no longer accept overlapping trait implementations that only differ in how the lifetime was bound.][72493] - [Rustc now correctly relates the lifetime of an existential associated type.][71896] This fixes some edge cases where `rustc` would erroneously allow you to pass a shorter lifetime than expected. - [Rustc now dynamically links to `libz` (also called `zlib`) on Linux.][74420] The library will need to be installed for `rustc` to work, even though we expect it to be already available on most systems. - [Tests annotated with `#[should_panic]` are broken on ARMv7 while running under QEMU.][74820] - [Pretty printing of some tokens in procedural macros changed.][75453] The exact output returned by rustc's pretty printing is an unstable implementation detail: we recommend any macro relying on it to switch to a more robust parsing system. [75453]: rust-lang/rust#75453 [74820]: rust-lang/rust#74820 [74420]: rust-lang/rust#74420 [74109]: rust-lang/rust#74109 [74150]: rust-lang/rust#74150 [73862]: rust-lang/rust#73862 [73887]: rust-lang/rust#73887 [73466]: rust-lang/rust#73466 [73516]: rust-lang/rust#73516 [73293]: rust-lang/rust#73293 [73007]: rust-lang/rust#73007 [73032]: rust-lang/rust#73032 [72920]: rust-lang/rust#72920 [72569]: rust-lang/rust#72569 [72583]: rust-lang/rust#72583 [72584]: rust-lang/rust#72584 [72717]: rust-lang/rust#72717 [72437]: rust-lang/rust#72437 [72445]: rust-lang/rust#72445 [72486]: rust-lang/rust#72486 [72493]: rust-lang/rust#72493 [72331]: rust-lang/rust#72331 [71896]: rust-lang/rust#71896 [71660]: rust-lang/rust#71660 [71322]: rust-lang/rust#71322 [70740]: rust-lang/rust#70740 [cargo/8270]: rust-lang/cargo#8270 [cargo/8325]: rust-lang/cargo#8325 [cargo/8387]: rust-lang/cargo#8387 [`Option::zip`]: https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.zip [`vec::Drain::as_slice`]: https://doc.rust-lang.org/stable/std/vec/struct.Drain.html#method.as_slice

Pkgsrc changes: * Portability patches for Illumos have been intregrated upstream, so are no longer needed in pkgsrc. * Adjust one other patch, and update vendor/libc cargo checksum. Upstream changes: Version 1.46.0 (2020-08-27) ========================== Language -------- - [`if`, `match`, and `loop` expressions can now be used in const functions.] [72437] - [Additionally you are now also able to coerce and cast to slices (`&[T]`) in const functions.][73862] - [The `#[track_caller]` attribute can now be added to functions to use the function's caller's location information for panic messages.][72445] - [Recursively indexing into tuples no longer needs parentheses.][71322] E.g. `x.0.0` over `(x.0).0`. - [`mem::transmute` can now be used in static and constants.][72920] **Note** You currently can't use `mem::transmute` in constant functions. Compiler -------- - [You can now use the `cdylib` target on Apple iOS and tvOS platforms.][73516] - [Enabled static "Position Independent Executables" by default for `x86_64-unknown-linux-musl`.][70740] Libraries --------- - [`mem::forget` is now a `const fn`.][73887] - [`String` now implements `From<char>`.][73466] - [The `leading_ones`, and `trailing_ones` methods have been stabilised for all integer types.][73032] - [`vec::IntoIter<T>` now implements `AsRef<[T]>`.][72583] - [All non-zero integer types (`NonZeroU8`) now implement `TryFrom` for their zero-able equivalent (e.g. `TryFrom<u8>`).][72717] - [`&[T]` and `&mut [T]` now implement `PartialEq<Vec<T>>`.][71660] - [`(String, u16)` now implements `ToSocketAddrs`.][73007] - [`vec::Drain<'_, T>` now implements `AsRef<[T]>`.][72584] Stabilized APIs --------------- - [`Option::zip`] - [`vec::Drain::as_slice`] Cargo ----- Added a number of new environment variables that are now available when compiling your crate. - [`CARGO_BIN_NAME` and `CARGO_CRATE_NAME`][cargo/8270] Providing the name of the specific binary being compiled and the name of the crate. - [`CARGO_PKG_LICENSE`][cargo/8325] The license from the manifest of the package. - [`CARGO_PKG_LICENSE_FILE`][cargo/8387] The path to the license file. Compatibility Notes ------------------- - [The target configuration option `abi_blacklist` has been renamed to `unsupported_abis`.][74150] The old name will still continue to work. - [Rustc will now warn if you have a C-like enum that implements `Drop`.][72331] This was previously accepted but will become a hard error in a future release. - [Rustc will fail to compile if you have a struct with `#[repr(i128)]` or `#[repr(u128)]`.][74109] This representation is currently only allowed on `enum`s. - [Tokens passed to `macro_rules!` are now always captured.][73293] This helps ensure that spans have the correct information, and may cause breakage if you were relying on receiving spans with dummy information. - [The InnoSetup installer for Windows is no longer available.][72569] This was a legacy installer that was replaced by a MSI installer a few years ago but was still being built. - [`{f32, f64}::asinh` now returns the correct values for negative numbers.] [72486] - [Rustc will no longer accept overlapping trait implementations that only differ in how the lifetime was bound.][72493] - [Rustc now correctly relates the lifetime of an existential associated type.][71896] This fixes some edge cases where `rustc` would erroneously allow you to pass a shorter lifetime than expected. - [Rustc now dynamically links to `libz` (also called `zlib`) on Linux.][74420] The library will need to be installed for `rustc` to work, even though we expect it to be already available on most systems. - [Tests annotated with `#[should_panic]` are broken on ARMv7 while running under QEMU.][74820] - [Pretty printing of some tokens in procedural macros changed.][75453] The exact output returned by rustc's pretty printing is an unstable implementation detail: we recommend any macro relying on it to switch to a more robust parsing system. [75453]: rust-lang/rust#75453 [74820]: rust-lang/rust#74820 [74420]: rust-lang/rust#74420 [74109]: rust-lang/rust#74109 [74150]: rust-lang/rust#74150 [73862]: rust-lang/rust#73862 [73887]: rust-lang/rust#73887 [73466]: rust-lang/rust#73466 [73516]: rust-lang/rust#73516 [73293]: rust-lang/rust#73293 [73007]: rust-lang/rust#73007 [73032]: rust-lang/rust#73032 [72920]: rust-lang/rust#72920 [72569]: rust-lang/rust#72569 [72583]: rust-lang/rust#72583 [72584]: rust-lang/rust#72584 [72717]: rust-lang/rust#72717 [72437]: rust-lang/rust#72437 [72445]: rust-lang/rust#72445 [72486]: rust-lang/rust#72486 [72493]: rust-lang/rust#72493 [72331]: rust-lang/rust#72331 [71896]: rust-lang/rust#71896 [71660]: rust-lang/rust#71660 [71322]: rust-lang/rust#71322 [70740]: rust-lang/rust#70740 [cargo/8270]: rust-lang/cargo#8270 [cargo/8325]: rust-lang/cargo#8325 [cargo/8387]: rust-lang/cargo#8387 [`Option::zip`]: https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.zip [`vec::Drain::as_slice`]: https://doc.rust-lang.org/stable/std/vec/struct.Drain.html#method.as_slice

rust-highfive assigned shepmaster May 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2020

rust-highfive assigned cramertj and unassigned shepmaster May 23, 2020

jonas-schievink added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 23, 2020

Ralith force-pushed the asinh-fix branch 2 times, most recently from 5e7bdd2 to c3b02de Compare May 23, 2020 21:39

Ralith force-pushed the asinh-fix branch from c3b02de to d1cdca7 Compare May 23, 2020 22:01

tspiteri reviewed May 24, 2020

View reviewed changes

src/libstd/f64.rs Outdated Show resolved Hide resolved

src/libstd/f32.rs Outdated Show resolved Hide resolved

Ralith force-pushed the asinh-fix branch from d1cdca7 to 8f8f560 Compare May 24, 2020 07:02

Ralith force-pushed the asinh-fix branch from 8f8f560 to 730f736 Compare May 24, 2020 07:03

Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2020

dtolnay approved these changes Jun 19, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2020

dtolnay added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 19, 2020

dtolnay reviewed Jun 19, 2020

View reviewed changes

src/libstd/f32.rs Outdated Show resolved Hide resolved

Remove now-redundant branch

35a2915

RalfJung mentioned this pull request Jun 19, 2020

Rollup of 13 pull requests #73498

Merged

bors merged commit 99be102 into rust-lang:master Jun 19, 2020

robojumper mentioned this pull request Jun 23, 2020

asinh of smallish value is infinite #73642

Closed

dtolnay assigned dtolnay and unassigned cramertj Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix asinh of negative values #72486

Fix asinh of negative values #72486

Ralith commented May 23, 2020 •

edited

Loading

rust-highfive commented May 23, 2020

ghost commented May 23, 2020

shepmaster commented May 23, 2020

Ralith commented May 23, 2020

Ralith commented Jun 18, 2020

Mark-Simulacrum commented Jun 19, 2020

dtolnay left a comment •

edited

Loading

dtolnay commented Jun 19, 2020

bors commented Jun 19, 2020

dtolnay commented Jun 19, 2020

bors commented Jun 19, 2020

RalfJung commented Jun 19, 2020

Fix asinh of negative values #72486

Fix asinh of negative values #72486

Conversation

Ralith commented May 23, 2020 • edited Loading

rust-highfive commented May 23, 2020

ghost commented May 23, 2020

shepmaster commented May 23, 2020

Ralith commented May 23, 2020

Ralith commented Jun 18, 2020

Mark-Simulacrum commented Jun 19, 2020

dtolnay left a comment • edited Loading

Choose a reason for hiding this comment

dtolnay commented Jun 19, 2020

bors commented Jun 19, 2020

dtolnay commented Jun 19, 2020

bors commented Jun 19, 2020

RalfJung commented Jun 19, 2020

Ralith commented May 23, 2020 •

edited

Loading

dtolnay left a comment •

edited

Loading