-
Notifications
You must be signed in to change notification settings - Fork 308
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
BaseArray::abs(&self) isn't implemented for integers (isize, i64, etc) #1462
Comments
So I think the general approach up to now has been that this can be accomplished via |
If I'm not mistaken, I believe that's the exact way it's being done for the floats using the #[cfg(feature = "std")]
macro_rules! unary_ops {
($($(#[$meta:meta])* fn $id:ident)+) => {
$($(#[$meta])*
#[must_use = "method returns a new array and does not mutate the original value"]
pub fn $id(&self) -> Array<A, D> {
self.mapv(A::$id)
})+
};
} Maybe some of the methods such as What's bugging me is the issue of partial functionality. Why should this method only work with floats when it affects negative numbers as a whole? If it the functionality is not added to integers, we should maybe update the documentation to make it clearer since, although the |
Honestly I agree, and would welcome a PR that documents that. If you're up for it, what I'd love to see is a PR that modifies the implementation of element-wise mathematics to mimic I have some code laying around that starts that more comprehensive PR, but feel that I've got a few items I've got to check off first. |
@Sh3mm you've inspired me to go back and take a look at that code. I think it's realistic for me to implement the more comprehensive PR. However, I'd like for the methods with additional arguments to accept both scalars and arrays, and I've got four ways to do it. Which of these do you think you'd prefer?
Pros: Absolutely no ambiguity in method resolution. Cons: Verbose, and when methods have additional arguments (e.g.,
Pros: When only using one of the traits, no ambiguity. Cons: When using both kinds in the same scope, requires specifying the method via fully-qualified syntax, and you still can't mix scalars and arrays. Also, you'd have to import the appropriate trait.
Pros: No method ambiguity, no proliferation of method names, and you can mix arrays and scalars. Cons: arguments are now verbose.
Pros: No proliferation of method names and you can succinctly mix arrays and scalars. Cons: Complex implementation means that compiler hints are less helpful when something does go wrong. Seems to work for almost all methods, but for some reason mimicking I am currently leaning strongly towards (4), but I am curious to get your take. |
Hum, here are my toughs about each way of doing thing:
I enjoy the simplicity, however, It might make it confusing for the users since they might not understand the difference between the two version just by reading the name. I think it's a nice way to do things, but I would also make sure to link the
I'm a bit uncomfortable with this implementation. Importing the trait when needed is one thing, but it's not rare to do operation with both scalars and array in the same code. I would not like to be in the situation where I have an existing code base using the
Frankly, this one feels a bit like a hack, but it feels to me like the best compromise of library management & ease of use.
First, I haven't really used const-generic, so I'm unsure of implementation details. However, based on your pros/cons, here's how I feel: Overall, I would tend towards (3) or (4). However, depending on philosophy, (1) might be good. The only one I feel negatively about would be the (2). |
The
abs(&self)
method inBaseArray
only works for float arrays since it's defined as part of the Element-wise methods for float arrays section.However, in my opinion, we should also be able to get the absolute version of negative integers arrays.
The text was updated successfully, but these errors were encountered: