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

Replace LinalgScalar by num_traits::Num #499

Closed
termoshtt opened this issue Oct 7, 2018 · 4 comments
Closed

Replace LinalgScalar by num_traits::Num #499

termoshtt opened this issue Oct 7, 2018 · 4 comments

Comments

@termoshtt
Copy link
Member

ndarray::linalg_traits::LinalgScalar and num_traits::Num are basically same trait. num-traits 0.2 has more specific traits i.e. Num, NumOps, NumRef, NumAssign, NumAssignOps and NumAssignRef. LinalgScalar is merely used in this crate, and it exists for user convenience. Should we replace LinalgScalar by Num?

@jturner314
Copy link
Member

ndarray::LinalgScalar requires the following: 'static + Copy + Zero + One + Add + Sub + Mul + Div. I think we could probably eliminate the Copy bound, but we can't really get rid of the 'static bound right now. (See rust-ndarray/ndarray-stats#3 for the reasoning for keeping 'static.)

num_traits::Num requires the following: PartialEq + Zero + One + Add + Sub + Mul + Div + Rem.

So, we could replace LinalgScalar bounds with 'static + Copy + Num, or possibly 'static + Num. To be honest, though, I don't really see any reason to do so. LinalgScalar is automatically implemented for T where T meets the required bounds, so it's not an issue of convenience. If we switch to 'static + Copy + Num, we're adding unnecessary Num + Rem bounds (and note that Num has to be implemented for the specific type, unlike LinalgScalar which is implemented for T).

Is there any reason in particular to make this change?

@termoshtt
Copy link
Member Author

Your comment for 'static is quite reasonable.

Is there any reason in particular to make this change?

The generalization of number types is responsible for the num-traits crate. IMO, we should not introduce a trait for fine-tuned minimal requirement (LinalgScalar) rather than using commonly used trait ('static + Num).

If we switch to 'static + Copy + Num, we're adding unnecessary Num + Rem bounds

This is actually problematic...
num-complex::Complex had been lacking these traits long time until rust-num/num#327. Num is not a large problem, but Rem is sometimes hard to implement.

@jturner314
Copy link
Member

We could use 'static + Copy + NumOps to avoid the Num requirement, but NumOps still requires Rem. It looks like all of the relevant num-traits traits require Rem.

@termoshtt
Copy link
Member Author

I make sense the difference of LinalgScalar and Num. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants