arrayfire / arrayfire-rust

Rust wrapper for ArrayFire
BSD 3-Clause "New" or "Revised" License
815 stars 58 forks source link

Improve generics usability for `Array<T>` #264

Open atomicky opened 3 years ago

atomicky commented 3 years ago

I encountered difficulty when writing generic functions using Array<T> e.g.

fn test_basic_op<T>(a: &af::Array<T>)
where
    // T: af::Fromf64 + af::HasAfEnum + ???
{
    println!("Element-wise arithmetic");
    let b = af::add(&af::sin(a), &T::fromf64(1.5f64), false);

    let b2 = af::add(&af::sin(a), &af::cos(a), false);

    let b3 = !a;
    af::af_print!("sin(a) + 1.5 => ", b);
    af::af_print!("sin(a) + cos(a) => ", b2);
    af::af_print!("!a => ", b3);

    let a_plus_b = a.clone() + b.clone();
    af::af_print!("a + b", a_plus_b);

    let minus_a = -(a.clone());
    af::af_print!("-a ", minus_a);
}

After long fight with compile error, I found that

fn test_basic_op<T>(a: &af::Array<T>)
where
    T: af::HasAfEnum<UnaryOutType = T>
        + af::Fromf64
        + af::Convertable<OutType = T>
        + af::ImplicitPromote<T, Output = T>,
    af::Array<T>: std::ops::Neg<Output = af::Array<T>>,
{
    ...
}

compiles and it's OK.

However, I feel that:

So I want:

  1. Alias-like trait (e.g. AfScalar: HasAfEnum + ...) which enables us ignoring implementation detail when using Array<T> in generic code
  2. Generic trait implementation (e.g. Convertable), not implementation for f32, f64 and other types individually

What are your thoughts?

9prady9 commented 3 years ago

Are the traits just implementation detail ?

I don't think they are just implementation detail. Generic Array trait bounds help with validating the input type statically at compile time. They also help establish what is expected input when a given set of functions are used inside a generic function. The other disadvantage of removing all these trait checks is ArrayFire functions (upstream library) calls will throw a runtime error when invalid inputs are passed to them.

About automatic trait bound addition ?

I am not sure if that is something possible in rust, but I will look into it. I think a comprehensive trait impls for a limited set of combinations should solve the problem for some of these traits. But such brute trick won't help for all cases.

Trait Aliases

That is not a stable feature in rust yet and more importantly it is impractical to define trait aliases to all combinations of traits for user defined generic functions - there is no practical way to know what functions constitute a given user defined generic function.

Generic trait implementation (e.g. Convertable), not implementation for f32, f64 and other types individually.

I don't understand the question. Convertable is one trait that is internal detail but it had to exposed to enable writing generic functions on user end. Ideally and in almost all cases, users are not meant to implement this trait over any other user defined type.

I understand writing a generic function with so many trait bounds takes time to figure out the correct bound check. Let me see what can be done to improve the situation.

atomicky commented 3 years ago

Thanks for your reply :smiley:

I'm sorry, I didn't explain enough. You're right, traits are NOT implementation detail. Compile-time errors are much better than runtime errors.

However, I think AfFloat below will help users.

use arrayfire as af;
use num;

// alias-like trait (NOTE: This code does not require nightly.)
trait AfFloat:
    num::Float
    + af::HasAfEnum<UnaryOutType = Self>
    + af::Convertable<OutType = Self>
    + af::ImplicitPromote<Self, Output = Self>
{
}

impl<T> AfFloat for T where
    T: num::Float
        + af::HasAfEnum<UnaryOutType = T>
        + af::Convertable<OutType = T>
        + af::ImplicitPromote<Self, Output = T>
{
}

fn test_basic_op<T>(a: &af::Array<T>)
where
    T: AfFloat + af::Fromf64,
    af::Array<T>: std::ops::Neg<Output = af::Array<T>>,
{
    ...
}

I wish af::FloatingPoint or af::RealFloating would be used like this. Can we do this?

atomicky commented 3 years ago

Question 2. was about implementation of Convertable using generics, not macros for each types. I wrote that and send PR. I don't know so much about type safety in arrayfire and some trait bounds may not be enough for safety, but I hope you'll get the idea.

9prady9 commented 3 years ago

I was about to add a modified example of the code stub from your original description to show why I think such usage of traits: Convertable and ImplicitPromote for Array<T> is incorrect.

I have seen the PR and will take a closer look at the generic changes and leave further feedback if required. Thank you for using and contributing to arrayfire-rust!

9prady9 commented 3 years ago

https://github.com/arrayfire/arrayfire-rust/pull/265#issuecomment-739799995

9prady9 commented 3 years ago

~I think the recent change of Convertible has an unexpected issue with different kind of function. May be it should be reverted but I am not sure yet. Will post an update soon.~ Incorrect example code was used. please ignore.

9prady9 commented 3 years ago

Leaving this as a place holder issue to improve generic programming using rust wrapper. Any future user, kindly showcase your use case if facing issues. We will try to improve as more features of traits in rust stabilize.

Note, I have added HasAfEnum trait bound to most of the other traits to avoid specification of HasAfEnum bound explicitly again in any of user code. Hopefully, that helps some cases.