Repository navigation
mapv_into_any doesn't respect storage type #1280
Description
Unlike mapv_into, mapv_into_any currently always returns a heap-allocated Array even if the source is something else - in my case, ArcArray.
It would be great if, like mapv_into, it was generic over storage types and allowed to clone ArcArray in-place when no copy is necessary.
cc @benkay86 who added it in the first place
Activity
benkay86 commentedon Apr 27, 2023
I didn't think about
ArcArrayas a use case formapv_into_any()when I contributed that PR, but I agree it would be nice to avoid unnecessary clones. @RReverser, would you mind providing a brief code snippet showing what you want to achieve?mapv_into_any()must return an owned array, but I think it might be possible to be generic over its return type to return anArcArrayinstead of anArray.Also, BTW,
ndarrayappears to be no longer actively maintained, see #1272. Some simple pull requests might still get merged, but it's unclear if or when a new release will happen.bluss commentedon Apr 27, 2023
mapv_into_any has the same limitations as map, map_mut, and mapv and mapv_into is the odd one (!)
It would be nice to solve but you sort of get into type system limitations, don't you?
RReverser commentedon Apr 27, 2023
Just want to do
mapv_into_anyto convert items on anArcArraybut reuse the existingArcArrayif it's already same type. Same as what it does onmapv_into.Those methods don't take array by ownership so I don't think they're comparable.
Not in this case tbh. I think it can be generic over
DataMut, just likemapv_into, so ifArcArrayalready has only one owner & is the same type, then it's modified in-place and in any other case it creates a newArcArray.bluss commentedon Apr 27, 2023
Mapv_into_any does not use the same signature as mapv_into, because the element type changes. We get into the general Rust problem of translating "
Foo<X>" into "Foo<Y>" (higher kinded types?). To us, they are both kinds of "Foo". But to Rust they are two unrelated and different types.You might see that the incoming storage type is "S" in
ArrayBase<S, ..>but the outgoing storage type will be something elseS2 != Sif the element types are not equal (A != B) because S depends on the element type.If we look elsewhere in Rust, "Vec" is not a type, but "
Vec<T>" for a particular T is a type.We know how to do some array kind-preserving but element mapping operations, we have https://docs.rs/ndarray/latest/ndarray/struct.ArrayBase.html#method.assume_init which does this after all, but I think it's kind of awkward. Would be great to see what the solution would look like here.
RReverser commentedon Apr 27, 2023
Sure, but
mapv_into_anyalready works around that by comparingTypeIds of the element. I don't see, however, how that restricts putting result storage repr to be the same as input - the implementation ofmapv_into_anydoesn't seem to restrict that in any way.bluss commentedon Apr 29, 2023
If you don't want my input on where challenges might lie - I've worked with these kinds of issues in ndarray before after all, when writing most of the crate - then maybe attempting an implementation and seeing what works in practice is a good way to go.
RReverser commentedon Apr 29, 2023
That... sounds unnecessarily hostile, but sure, I'm happy to give it a stab. I just thought @benkay86 might be more interested hence the issue.
RReverser commentedon Apr 29, 2023
Ah and yeah I see what you meant, but now that we have higher-kinder associated types I think K this might be still doable. If not, another type param could be just arbitrary storage destination.
The bigger problem is that one way or another this would be a breaking change at this point, as someone might rely on output being always Array, so I guess this would require a yet another separate function.
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
benkay86 commentedon May 3, 2023
@bluss, happy to see you active on GitHub! Ndarray is a really valuable project, happy to see it hasn't been completely abandoned.
OK, I took a stab at this with #1283. Looks OK except for pre-existing issues with CI and clippy.
This is a breaking change that will require users to call
mapv_into_any()differently. Currentlymapv_into_any()always returns anArray, so there is no need for the caller to annotate the return type. #1283 makesmapv_into_any()generic over a return type ofArrayBase<T: DataOwned, _>, so the caller will need to be explicit about whether it should return anArrayor anArcArray. I don't think this is a big deal -- it's pretty similar to howIterator::collect()works. But it may require a 0.16 release since it is a breaking change.Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
RReverser commentedon May 3, 2023
@benkay86 This is great, thanks! It's pretty much what I had in mind with making it generic over return type but I had the same concerns about backward compatibility so wanted to hear back on those first. Appreciate you implementing it though!
RReverser commentedon May 3, 2023
FWIW another option I was thinking about was just moving this method to specialised
impls, that is, having individual methodsmapv_into_anyonArray, separate onArcArrayetc.This would allow the caller to avoid specifying return type as a param, always returning same storage type as input, but would make it less flexible so not sure it's worth it.
bluss commentedon May 3, 2023
I don't quite have the bandwidth. I'd like to bring in new people to maintain, if it can be done.
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280
Make mapv_into_any() work for ArcArray, resolves rust-ndarray#1280