BaseArray::abs(&self) isn't implemented for integers (isize, i64, etc) #1462

Activity

1. akern40

   added

   enhancement

   on Dec 5, 2024
2. akern40 commented on Dec 5, 2024

   

   akern40

   on Dec 5, 2024

   Collaborator

   More actions

   So I think the general approach up to now has been that this can be accomplished via arr.mapv(num_traits::abs) or arr.mapv(i64::abs) if you want have / want more specificity. I would maybe like to see a sort of parallel implementation of num-like traits for arrays, but this gets back to the problem of how many methods are in ndarray.
3. Sh3mm commented on Dec 5, 2024

   

   Sh3mm

   on Dec 5, 2024

   Last edited by Sh3mm

   Author

   More actions

   If I'm not mistaken, I believe that's the exact way it's being done for the floats using the unary_ops macro.

   #[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 absand signum could be moved to a different section/macro that would treat both cases?

   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 abs method is in the Element-wise methods for float arrays section, it is far enough into it that the section name does not appear on screen when navigating through the side menu. This makes it unclear that it only applies to floats when quickly browsing the docs.
4. akern40 commented on Dec 7, 2024

   

   akern40

   on Dec 7, 2024

   Collaborator

   More actions

   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 num, so that if a type is num_traits::Signed it will have abs and signum.

   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.

   👍React with 👍1Reacted by Sh3mm
5. akern40 commented on Dec 16, 2024

   

   akern40

   on Dec 16, 2024

   Collaborator

   More actions

   @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?

   1. Implement them as separate methods, abs_sub, abs_sub_mut, abs_sub_scalar, and abs_sub_scalar_mut

   Pros: Absolutely no ambiguity in method resolution. Cons: Verbose, and when methods have additional arguments (e.g., mul_add) you can't mix scalars and arrays.

   2. Use two traits, e.g., SignedScalar and SignedArr, that have overlapping method names abs_sub and abs_sub_mut

   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.

   3. Use only two methods, but allow the additional arguments to be passed as wrapped enums, e.g., arr.abs_sub(Arg::Arr(1.0))

   Pros: No method ambiguity, no proliferation of method names, and you can mix arrays and scalars. Cons: arguments are now verbose.

   4. Use a const-generic-based approach that lets me implement just two methods, abs and abs_mut, without needing the enums.

   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 num::Pow requires me to essentially fall back to one of the above options.

   I am currently leaning strongly towards (4), but I am curious to get your take.
6. Sh3mm commented on Dec 16, 2024

   

   Sh3mm

   on Dec 16, 2024

   Last edited by Sh3mm

   Author

   More actions

   Hum, here are my toughs about each way of doing thing:

   1. Implement them as separate methods, abs_sub, abs_sub_mut, abs_sub_scalar, and abs_sub_scalar_mut

   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 scalar version in the doc of the array one and vice versa.

   2. Use two traits, e.g., SignedScalar and SignedArr, that have overlapping method names abs_sub and abs_sub_mut

   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 SignedScalar and having to comeback and modifying all previous uses of the abs_sub method to their fully-qualified syntax if I suddenly need the SignedArr version.

   3. Use only two methods, but allow the additional arguments to be passed as wrapped enums, e.g., arr.abs_sub(Arg::Arr(1.0))

   Frankly, this one feels a bit like a hack, but it feels to me like the best compromise of library management & ease of use.

   4. Use a const-generic-based approach that lets me implement just two methods, abs and abs_mut, without needing the enums.

   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:
   From a maintainer point of view, I dislike exceptions, so the num::Pow would royally tick me off. The complexity might be a problem, but since I do not understand how you'd do it, I refrain judgment.
   As a user, I feel like the hint issue is easily manageable with a good doc.

   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).

   ❤️React with ❤️1Reacted by akern40