num-traits icon indicating copy to clipboard operation
num-traits copied to clipboard

No `.abs()` on integers, any reason?

Open amandasaurus opened this issue 6 years ago • 12 comments

There are some method on Float which aren't on any of the integer traits (PrimInt? Num Itself). e.g. .abs(). A i32 can defintely have an .abs() method. (.abs() on a u8 makes sense (ish)). Something I'm doing would benefit from more generic numeric traits.

Is there any reason why Float has methods which could be applied to more generic numbers? I don't want to submit a patch if there's some good reason I'm not aware of

amandasaurus avatar Apr 23 '18 20:04 amandasaurus

There is Signed::abs(). You're right that abs() (and probably all of Signed) could be trivially implemented for unsigned numbers too, but the trait is somewhat useful as a marker too.

cuviper avatar Apr 23 '18 20:04 cuviper

Would it make sense to split up the marker trait from operations involving sign? There could be a trait called SignOps or something like that, which contains all methods of Signed as it stands now. Then repurpose the Signed trait as a marker for signed vs unsigned types.

maxbla avatar Jul 21 '19 22:07 maxbla

We also tend to stick to methods that are inherently available on the types. (I won't claim this universally, as I'm sure there are exceptions, but just as a general tendency.) So on that note, the unsigned primitive integers don't provide abs() or any of the other Signed methods that would be const/no-op.

I'm wary of adding that, because it seems like a code-smell if you're doing signed-like operations on something unsigned. At the least, I'd like to see a real motivating case for this.

cuviper avatar Jul 22 '19 17:07 cuviper

Off the top of my head, I was just doing some work on formatting traits for Ratio<T> in the rust-rational crate. I wanted some way to tell if the numerator was negative, so I could add a - character. To do that, I settled on the following hack (for now)

let non_negative = !(self < &<Ratio<T> as Zero>::zero());

If all Integers (including unsigned ones) implemented SignOps with some sort of is_positive() or is_negative() functions, I wouldn't have to do that.

I think more generally that splitting up the traits would be useful for an algorithm that has to deal with sign explicitly, but that is generic across both signed and unsigned Integers.

I agree that abs() on an unsigned number is smelly, but I'm mostly after the is_positive and is_negative functions.

maxbla avatar Jul 22 '19 18:07 maxbla

OK, well... adding a new SignOps is no issue if we decide it is needed. Grafting it into the current trait hierarchy is a breaking change though.

cuviper avatar Jul 22 '19 18:07 cuviper

We could make this not a breaking change by having a SignOps trait that has a blanket implementation for Signed

maxbla avatar Jul 22 '19 18:07 maxbla

But you can't make it a supertrait of Integer without addressing the unsigned types too.

cuviper avatar Jul 22 '19 18:07 cuviper

Am I wrong to assume that you could have a blanket implementation for Signed types and bespoke (non-blanket?) implementations for all the unsigned Integers?

maxbla avatar Jul 22 '19 18:07 maxbla

I don't think that's possible without specialization.

cuviper avatar Jul 22 '19 18:07 cuviper

I don't fully understand the discussion above... I can only contribute by providing an example of why this feels like a problem and how google brought me here;

I am attempting to contribute to a library (geo) which is forcing me to work with a type (CoordFloat) constrained to num_traits::Float but not constrained to num_traits::Signed (I'm surprised but assuming there is a good reason that Floats aren't always Signed)

But now I want to use num_traits::abs() and I am getting an error. I am looking for a neat work around for this problem;

fn cross_product<T>(left: Coord<T>, right: Coord<T>) -> T
where
  T: CoordFloat,
{
  left.x * right.y - left.y * right.x
}

// ...

fn offset<T>(ls:LineString<T>, distance: T) -> LineString<T>
where 
  T : CoordFloat // Constrained to num_traits::Float
{
  // ...
  let ab:Coord<T> = *b - *a;
  let cd:Coord<T> = *d - *c;
  let my_value = cross_product(ab, cd);
  if num__traits::abs(my_value) < num_traits::cast(0.0000001f64).unwrap() { 
    //                ^^^^^^^^ doesn't work
    // ...
  }
  // ...
}

I can see a few undesirable solutions;

  • Add the constraint + num_traits::Signed to my functions. I don't feel that is right... the rest of the geo crate only constrains things to CoordFloat why cant my code get away with that?
  • use num_traits::abs_sub() inside my cross product function... but I don't want to do that. I rely on potentially negative outputs elsewhere.

Many thanks

thehappycheese avatar Nov 12 '22 05:11 thehappycheese

@thehappycheese can't you just use Float::abs? You even get method syntax, my_value.abs().

cuviper avatar Nov 21 '22 18:11 cuviper

@thehappycheese can't you just use Float::abs? You even get method syntax, my_value.abs().

Hi @cuviper, thanks for the response. Ah apologies yes that does work. I am not sure how I did not think to try that... Now I found 3 ways to do it, 2 of them work, 1 looks like it should but doesn't:

pub(super) fn line_intersection_with_parameter<T>(
    a: &Coord<T>,
    b: &Coord<T>,
    c: &Coord<T>,
    d: &Coord<T>,
) -> LineIntersectionWithParameterResult<T>
where
    T: CoordFloat,  // CoordFloat is constrained to num_traits Float but not Signed
{
    //    ...
    let ab_cross_cd:T = cross_product(ab, cd);

    // Works
    let test = T::abs(ab_cross_cd);
    // Works
    let test = ab_cross_cd.abs();
    // Does not work:
    let test = num_traits::abs(ab_cross_cd); // T is not Signed
    // ...
}

thehappycheese avatar Nov 22 '22 03:11 thehappycheese