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

mapv_into_any doesn't respect storage type #1280

Open
RReverser opened this issue Apr 27, 2023 · 12 comments
Open

mapv_into_any doesn't respect storage type #1280

RReverser opened this issue Apr 27, 2023 · 12 comments

Comments

@RReverser
Copy link

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

@benkay86
Copy link
Contributor

I didn't think about ArcArray as a use case for mapv_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 an ArcArray instead of an Array.

Also, BTW, ndarray appears 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
Copy link
Member

bluss commented 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
Copy link
Author

@RReverser, would you mind providing a brief code snippet showing what you want to achieve?

Just want to do mapv_into_any to convert items on an ArcArray but reuse the existing ArcArray if it's already same type. Same as what it does on mapv_into.

mapv_into_any has the same limitations as map, map_mut, and mapv and mapv_into is the odd one (!)

Those methods don't take array by ownership so I don't think they're comparable.

It would be nice to solve but you sort of get into type system limitations, don't you?

Not in this case tbh. I think it can be generic over DataMut, just like mapv_into, so if ArcArray already has only one owner & is the same type, then it's modified in-place and in any other case it creates a new ArcArray.

@bluss
Copy link
Member

bluss commented 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 else S2 != S if 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
Copy link
Author

To us, they are both kinds of "Foo". But to Rust they are two unrelated and different types.

Sure, but mapv_into_any already works around that by comparing TypeIds of the element. I don't see, however, how that restricts putting result storage repr to be the same as input - the implementation of mapv_into_any doesn't seem to restrict that in any way.

@bluss
Copy link
Member

bluss commented 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
Copy link
Author

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
Copy link
Author

higher kinded types?

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.

benkay86 added a commit to benkay86/ndarray that referenced this issue May 3, 2023
benkay86 added a commit to benkay86/ndarray that referenced this issue May 3, 2023
@benkay86
Copy link
Contributor

benkay86 commented 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. Currently mapv_into_any() always returns an Array, so there is no need for the caller to annotate the return type. #1283 makes mapv_into_any() generic over a return type of ArrayBase<T: DataOwned, _>, so the caller will need to be explicit about whether it should return an Array or an ArcArray. I don't think this is a big deal -- it's pretty similar to how Iterator::collect() works. But it may require a 0.16 release since it is a breaking change.

benkay86 added a commit to benkay86/ndarray that referenced this issue May 3, 2023
@RReverser
Copy link
Author

@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
Copy link
Author

so the caller will need to be explicit about whether it should return an Array or an ArcArray

FWIW another option I was thinking about was just moving this method to specialised impls, that is, having individual methods mapv_into_any on Array, separate on ArcArray etc.

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
Copy link
Member

bluss commented 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.

benkay86 added a commit to benkay86/ndarray that referenced this issue May 4, 2023
benkay86 added a commit to benkay86/ndarray that referenced this issue May 4, 2023
benkay86 added a commit to benkay86/ndarray that referenced this issue May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants