Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate {f32,f64}::abs_sub. #33664

Merged
merged 1 commit into from
May 23, 2016
Merged

Deprecate {f32,f64}::abs_sub. #33664

merged 1 commit into from
May 23, 2016

Conversation

huonw
Copy link
Member

@huonw huonw commented May 16, 2016

The abs_sub name is misleading: the function actually computes the
positive difference (fdim in C), not the (x - y).abs() that many people expect
from the name.

This function can be replaced with just (x - y).max(0.0), mirroring
the abs version, but this behaves differently with NAN: NAN.max(0.0) == 0.0, while NAN.positive_diff(0.0) == NAN. People who absolutely
need that behaviour can use the C function directly and/or talk to the libs
team (we haven't encountered a concrete use-case for this functionality).

Closes #30315.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@huonw
Copy link
Member Author

huonw commented May 16, 2016

cc @rust-lang/libs

This differs to the decision we made in the meeting last week: we decided to just deprecate the function, recommending the use of (x - y).max(0.0) if positive-difference is the operation one wants, however this unfortunately differs to C's fdim and the existing abs_sub in how it handles NAN. Thus, I've introduced positive_diff as a renaming.

@sfackler
Copy link
Member

Do we actually know of any contexts in which this operation is used?

@huonw
Copy link
Member Author

huonw commented May 16, 2016

I don't know of any, but it seems unfortunate to deprecate a function and recommend something that is not a true replacement: it's happened before, and people rightly haven't been happy. If people are happy to take this "risk", I'll just remove the new functions in line with what we decided last week, but we didn't think about NaN then.

(FWIW, I found some more history about this operation: the fdim name in C comes from the Fortran DIM operation, which is sometimes styled DiM and apparently was implemented something like x - min(x, y) historically, suggesting the name comes from "difference from minimum".)

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 16, 2016
@alexcrichton
Copy link
Member

Sounds reasonable to me!

/// assert!(abs_difference_x <= f32::EPSILON);
/// assert!(abs_difference_y <= f32::EPSILON);
/// ```
#[stable(feature = "rust1", since = "1.10.0")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature name won’t work I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It compiles, what do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, @alexcrichton pointed out that make tidy fails. Thanks @nagisa.

pub fn abs_sub(self, other: f64) -> f64 {
unsafe { cmath::fdim(self, other) }
}

/// The positive difference of two numbers.
///
/// * If `self <= other`: `0:0`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0:0 or 0.0?

The abs_sub name is misleading: the function actually computes the
positive difference (`fdim` in C), not the `(x - y).abs()` that *many* people expect
from the name.

This function can be replaced with just `(x - y).max(0.0)`, mirroring
the `abs` version, but this behaves differently with NAN: `NAN.max(0.0)
== 0.0`, while `NAN.positive_diff(0.0) == NAN`. People who absolutely
need that behaviour can use the C function directly and/or talk to the libs
team (we haven't encountered a concrete use-case for this functionality).

Closes rust-lang#30315.
@huonw huonw changed the title Deprecate {f32,f64}::abs_sub, renaming to positive_diff. Deprecate {f32,f64}::abs_sub. May 16, 2016
@huonw
Copy link
Member Author

huonw commented May 16, 2016

Updated to just deprecate the functions, including calling for people to file issues describing their use-cases for positive difference if they have them.

@huonw
Copy link
Member Author

huonw commented May 16, 2016

(Alternatively, I could just point them to #30315 or this PR and ask for a comment?)

@alexcrichton
Copy link
Member

The libs team discussed this PR during triage yesterday and the conclusion was:

  • Let's just deprecate this function because it's misleading
  • Let's not add a replacement as the functionality seems very niche for the exact handling of NaN
  • Let's land after the next beta is cut to have an extended deprecation period.

@alexcrichton
Copy link
Member

@bors: r+ eb67f49

Beta is cut!

@bors
Copy link
Contributor

bors commented May 23, 2016

⌛ Testing commit eb67f49 with merge 57ef015...

bors added a commit that referenced this pull request May 23, 2016
Deprecate {f32,f64}::abs_sub.

The abs_sub name is misleading: the function actually computes the
positive difference (`fdim` in C), not the `(x - y).abs()` that *many* people expect
from the name.

This function can be replaced with just `(x - y).max(0.0)`, mirroring
the `abs` version, but this behaves differently with NAN: `NAN.max(0.0)
== 0.0`, while `NAN.positive_diff(0.0) == NAN`. People who absolutely
need that behaviour can use the C function directly and/or talk to the libs
team (we haven't encountered a concrete use-case for this functionality).

Closes #30315.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants