Implement arithmetic ops on more combinations of types

[jturner314]

In particular, this adds implementations for ArrayView where&ArrayBase was allowed previously, and it adds support for in-place operations where the RHS is any array (instead of requiring the RHS to be a reference).

This functionality seems useful. It doesn't allow anything that wasn't previously possible by adding a & in front of a parameter, but removing the requirement of a & reduces verbosity and may be more friendly to new users.

Thoughts?

See also #237 and #743.

[LukeMathWalker]

I agree that it reduces verbosity and accidental complexity: you are not left puzzled trying to understand why only certain combinations are allowed.
On the other hand, I think I remember @bluss being concerned this could lead to code bloat/sensibly longer compile times.

[jamestwebber]

As the initiator of #743 I will say that I just straight-up missed the relevant documentation (and/or didn't properly grok it) and while adding this feature would have saved me a little time I wouldn't want you to trade nice code or fast compiles just to have it.

Looking at the documentation now I can't figure what went wrong. Maybe I was thrown off by the use of @ to mean "any binary operator". Making that section more verbose might be a simpler and sufficient solution.

[bluss]

The summary issue is #699 where we coordinate work on this - we definitely want to improve this in some way.

I'd investigate what's the total count of arith ops impls before and after this change? What's the clean build time of the ndarray crate before and after?

One suggested strategy for improving compilation time given in #699 is to have fewer impl blocks that cover more cases. We can use marker traits if needed. (We can use https://doc.rust-lang.org/nightly/std/array/trait.LengthAtMost32.html as inspiration in a way - it's a marker that allows each other array trait impl to be written as one impl instead of 33 pieces.)

I think this should be seen in context of #699. Here we add more impls that do the same "clone whole array and then do the op", which is something we want to fix.

[bluss]

Treating ArrayView and &ArrayBase same seems sensible. So I'd say it would be great if those could be treated in the same impl.

[bluss]

Not important, but just something I saw. Allegedly this doc is not visible in rustdoc, but that comment may be outdated now(!). then we want that #[doc=$doc] back to make a complete sentence.

[bluss]

I would especially discourage these impl blocks. We have to spend a lot of impl blocks on the arithmetic ops with scalars on the left hand side. Part of me want to just remove everything with left hand side scalars, because it can never(?) be properly general because of how we need to express it as an impl .. for $scalar (for a specific single type).

[jturner314]

I've removed this impl block in the most recent commit because it's not necessary.

Part of me want to just remove everything with left hand side scalars, because it can never(?) be properly general because of how we need to express it as an impl .. for $scalar (for a specific single type).

I understand the objection, and AFAIK it will never be possible to write these impls for generic scalars even after re_rebalance_coherence. However, IMO supporting left-hand-side scalars is important, even if it's just f32/f64. I use these impls all the time in my code. They are especially important for non-commutative ops, where it's not straightforward to just move the scalar to the other side.

[bluss]

Thanks for updating this, even if it's a draft PR or whatever, it makes it easy to discuss changes 🙂

[jturner314]

@bluss's comments made me realize that we don't actually need separate implementations for ArrayView; we can write implementations for arbitrary ArrayBase, using .into_owned() as necessary. This is no more expensive than the approach in ndarray 0.13 (which requires DataOwned in many cases) because:

• For owned arrays, .into_owned() is a no-op.
• For ArcArrays, .into_owned() calls ensure_unique, which was called in the old implementation anyway because it was mutating the array.
• For ArrayViews (which weren't supported in the old implementation), the cost is equivalent to using &array. This is similarly true for ArrayViewMut and CowArray.

This is really nice. The only issue is that it's a breaking change because the return type is always Array now. (Before, some implementations would return ArrayBase<S, D>.)

[bluss]

It's nice to express, but it risks cementing one of the things we want to fix - excessive cloning?

I don't think it's right to change the return type of ArcArray<T> + T or similar. That should still be ArcArray. This is still the spirit of predictiveness. We want to preserve the kind of the array if we can, and we even want to preserve the layout of the array if we can (if the user uses F arrays, we return those where reasonable).

[bluss]

(Fwiw, there is a benchmark to compare the "excessive cloning vs not" and there isn't so much difference. The benchmarks are called add_2d_alloc_* and I'm not sure it gains so much. The "x.to_owned() + y" approach has one benefit of having simpler loops - a "memcpy" to copy, one binary operand loop to add in place; when we lose the extra cloning we have a ternary operand loop instead, to add x and y and store into z? But if both perform the same, maybe the latter should have the moral victory? And we can even lose the Clone bound, which I guess means we have to do it.)