tspiteri commented on Oct 14, 2021

For signed, I would prefer the name unsigned_abs_diff as it returns a different type, making it consistent with unsigned_abs.

orlp commented on Oct 14, 2021

For unsigned_abs a distinction needed to be made with a pre-existing abs function. We don't have to worry about that backwards compatibility here.

tspiteri commented on Oct 14, 2021

But I found i64::abs_diff returning u64 very surprising: a method that operates on two numbers to give a new number silently changes the type. The only other i64 method returning u64 is unsigned_abs, and its name clearly indicates that.

There are other methods that never return a negative value, and they still return the same type. For example i64::abs itself returns i64, not u64, even though it can overflow as well on signed (although only for one value).

shreepads commented on Dec 14, 2021 •

Current state:

u32vec.iter().fold(0, |acc, x| acc + (*x as i32 - median as i32).abs() as u32)

Desired state:

u32vec.iter().fold(0, |acc, x| acc + x.abs_diff(median))        // as per experimental API for u32

@shreepads I have done this:

fn abs_diff(a: u32, b: u32) -> u32 {
  if a > b {
    a - b
  } else {
    b - a
  }
}

Somehow this is cleaner to me than casting to signed. I think it's more efficient too? Not as nice as if it were a built-in method, but still cleans up your fold, no?

@tspiteri, if you want abs_diff to return an i64, what behavior do you suggest for the case when abs_diff can't fit in an i64? Panic? Change return type to result? I see no appealing options.

(The difference between two i64s can be 64 bits long, and i64 has 63 bits for magnitude)

tspiteri commented on Dec 21, 2021

@rrokkam I would expect the same overflow behaviour as abs itself.

nhamovitz commented on Dec 21, 2021

(FWIW, because I'm newish to Rust —)

I very much disagree — it doesn't make sense to me to ship an overflowing API when a 100% correct + infallible algorithm exists. I'll concede some case for naming it unsigned_abs_diff to highlight that the type is changing (although IMO that's what the function signature + your tooling is there for), but allowing overflows seems bad.

@shreepads I have done this:

fn abs_diff(a: u32, b: u32) -> u32 {
  if a > b {
    a - b
  } else {
    b - a
  }
}

Somehow this is cleaner to me than casting to signed. I think it's more efficient too? Not as nice as if it were a built-in method, but still cleans up your fold, no?

Thanks @MarcelRobitaille , yes that is much better and would work till such time as a method is available in the std library - (there is already such a method in the experimental API for u32)...

I was only hacking one of the Advent of Code puzzles (https://adventofcode.com/2021/day/7) so didn't mind the casting/ re-casting..

m-ou-se commented on Jan 9

@rfcbot merge

rfcbot commented on Jan 9 •

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

tspiteri commented on Jan 10

Has a final decision on the name abs_diff vs unsigned_abs_diff been made? I did have a use case where the input values were pretty small (pixel values with a limited range of 256) but then the output was used in an algorithm that required a larger signed range. In that case overflow was not an issue.

Of course, with no overflow there is no tricky case that requires this method, and (a - b).abs() is basically fine. I only brought this up in the first place because if an abs_diff method is present it kinda leads to using a.abs_diff(b) instead of (a - b).abs(), and then there will be an API signedness inconsistency between abs_diff and abs. Since (a - b).abs() is not too cumbersome, it may not be an issue, as long as a concious decision is made before stabilization.

rrokkam commented on Jan 11

The difference is that (a - b).abs() panics with underflow when b > a, and when b <= a the abs() call does nothing.

a.abs_diff(b) does not panic when b > a.

rfcbot commented 22 days ago

This is now entering its final comment period, as per the review above.

tspiteri commented 12 days ago

The difference is that (a - b).abs() panics with underflow when b > a, and when b <= a the abs() call does nothing.

This is for unsigned, while I was talking about signed.

rfcbot commented 12 days ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

CleanCut commented 11 days ago

@tspiteri Reading through this thread, I don't feel like there was a response explaining why your suggestion was not taken. In the spirit of collaboration, I'd like to attempt to fill in that gap according to my understanding (as an observer, not a decision maker) why abs_diff is an appropriate name!

Adding unsigned anywhere in a method name (usually in the suffix) is helpful to distinguish between a pair of analogous functions for both signed and unsigned flavors of the same operation. Otherwise, in general it is redundant to include signedness in the method name. You can see this (implicit?) naming policy if you examine the long list of methods on i32. Every method that includes unsigned in the name has a corresponding method with a signed flavor (which simply omit unsigned rather than containing signed). The rest of the methods do not mention the signedness in the name of the method at all, including the other almost two dozen methods which return unsigned values other than the one you cited (which does have an analogous signed method).

Anyway, just trying to help you see why I believe your naming suggestion wasn't selected. I could be wrong, in which case I welcome clarification. And, of course, you are welcome to continue preferring your own naming suggestion. I just wanted to make sure you felt heard.

m-ou-se commented 10 days ago

Has a final decision on the name abs_diff vs unsigned_abs_diff been made?

Sorry for the delayed response. We did quickly discuss this in a meeting, and concluded that we don't want a signed version of this, so there's no need for unsigned in the name. See @CleanCut's comment, who worded it better than I could have. :)