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

Improve generics usability for Array<T> #264

Open
atomicky opened this issue Nov 28, 2020 · 7 comments
Open

Improve generics usability for Array<T> #264

atomicky opened this issue Nov 28, 2020 · 7 comments
Assignees
Milestone

Comments

@atomicky
Copy link
Contributor

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:

  • Convertable and ImplicitPromote seems implementation detail and I don't want to write them every time when using Array<T>.
  • T should be Convertable<OutType=T> automatically when T: HasAfEnum (i.e. T is supported in arrayfire).

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
Copy link
Member

9prady9 commented Nov 30, 2020

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
Copy link
Contributor Author

atomicky commented Nov 30, 2020

Thanks for your reply 😃

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
Copy link
Contributor Author

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
Copy link
Member

9prady9 commented Nov 30, 2020

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.

  • None of those two traits are meant to be used for defining bounds against Array itself, rather they are for the underlying type of Array.
  • About trait like alias(I understand they don't need nightly): I am still contemplating as to how helpful such aliases are in general. Also, associated types like UnaryOutType are not meant to be used in such a fashion as T: HasAfEnum<UnaryOutType = T> as that would further restrict the type of Arrays that can be provided as input to function. For example, only types whose UnaryOutType is same as Self will be allowed to this function which is not the intended use case of this or other associated types defined under HasAfEnum trait.
  • The reason I used a macro to impl Convertable is to restrict the implementation of that traits to the base types of Array generic and not do it for Array<T>.

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
Copy link
Member

9prady9 commented Dec 7, 2020

#265 (comment)

@9prady9
Copy link
Member

9prady9 commented Dec 8, 2020

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
Copy link
Member

9prady9 commented Dec 9, 2020

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.

@9prady9 9prady9 added this to the timeless milestone Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants