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

abs_sub is strangely named #30315

Closed
huonw opened this issue Dec 11, 2015 · 11 comments
Closed

abs_sub is strangely named #30315

huonw opened this issue Dec 11, 2015 · 11 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@huonw
Copy link
Member

huonw commented Dec 11, 2015

The C name is fdim, and the operation is described as "positive difference" by e.g. the C function's man page, which isn't really suggested by "abs_sub" at all.

@huonw huonw added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 11, 2015
@nagisa
Copy link
Member

nagisa commented Dec 11, 2015

I disagree abs_sub is strangely named. At least it is much better than fdim. While it could certainly be abs_difference, absolute value_subtraction is fine IMO.

@ranma42
Copy link
Contributor

ranma42 commented Dec 11, 2015

Note that abs_sub does not compute the absolute value of the difference abs(x-y), but instead max(x-y, 0).

@jFransham
Copy link
Contributor

@ranma42 abs(x) ≡ max(x, 0) (logically, even if not in implementation), so abs(x-y) ≡ max(x-y, 0)

EDIT: This comment is stupid and wrong and horribly embarrassing

@hanna-kruppe
Copy link
Contributor

@jFransham abs(-1) is 1, but max(-1, 0) is 0. Did you mean abs(x) = max(x, -x)? In any case, this playground demonstrates what @ranma42 stated.

@nagisa
Copy link
Member

nagisa commented Dec 11, 2015

Good point, I guess abs_sub indeed does not make any sense at all.

@GuillaumeGomez
Copy link
Member

I think one of the following names would fit better:

  • positive_difference
  • positive_diff
  • pos_diff (but I don't really like this one, too short IMO)

@nagisa
Copy link
Member

nagisa commented Jan 8, 2016

Monus as pointed out by sp3d on IRC. The page also mentions these names the operation is referred to:

  • truncated subtraction;
  • limited subtraction;
  • proper subtraction.

@nagisa
Copy link
Member

nagisa commented Jan 8, 2016

Most of the methods name the result (e.g. (10.0).log() stands for logarithm of 10.0), and only some name the operation (e.g. mul_add which stands for multiply-add). I’d prefer staying with the bunch of first bucket, which pretty much implies using difference for whatever comes in the 2nd part. So {}_difference or {}_diff.

@alexcrichton
Copy link
Member

The libs team discussed this issue during triage the other day and the conclusion was that this is so confusingly named and sparingly used that we should probably just document it. The deprecation message can indicate:

  • What you probably want, (a - b).abs(), is not what this function is doing and that code is shorter than calling the function.
  • If you want this, you probably don't want to call it via a misleading name, and (a - b).max(0) is probably clearer and has the same semantics.

@huonw
Copy link
Member Author

huonw commented May 11, 2016

(a - b).max(0) is probably clearer and has the same semantics.

After the meetings, I realised this isn't quite true: fdim(NAN, 1) == NAN.abs_sub(1) == NAN, while (NAN - 1).max(0) == 0.

@HeroesGrave
Copy link
Contributor

I just wasted somewhere between 3-4 hours over the last day debugging my game, thinking that this was equivalent to (a - b).abs().

I'm definitely in support of deprecation and renaming.

huonw added a commit to huonw/rust that referenced this issue 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 rust-lang#30315.
bors added a commit that referenced this issue 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.
mbattifarano added a commit to mbattifarano/rust-geo that referenced this issue Jun 20, 2017
`x.abs_sub(y)` is defined
([confusingly](rust-lang/rust#30315)) as
`max(x-y, 0)`, not `abs(x-y)`. This wasn't caught in tests since `t_x -
t_y` in
[src/algorithm/intersects.rs:48](https://github.com/georust/rust-geo/blob/master/src/algorithm/intersects.rs#L48)
is positive in all of the given examples.
bors bot added a commit to georust/geo that referenced this issue Jun 22, 2017
120: Incorrect usage of `abs_sub` r=frewsxcv

`x.abs_sub(y)` is defined ([confusingly](rust-lang/rust#30315)) as `max(x-y, 0)`, not `abs(x-y)`. This wasn't caught in tests since `t_x - t_y` in [src/algorithm/intersects.rs:48](https://github.com/georust/rust-geo/blob/master/src/algorithm/intersects.rs#L48) happens to be positive in all of the given examples so I've added a new example where `t_x - t_y` is negative.
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

No branches or pull requests

8 participants