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

Tracking Issue for array_zip #80094

Closed
1 of 2 tasks
usbalbin opened this issue Dec 16, 2020 · 35 comments · Fixed by #112096
Closed
1 of 2 tasks

Tracking Issue for array_zip #80094

usbalbin opened this issue Dec 16, 2020 · 35 comments · Fixed by #112096
Labels
A-array Area: [T; N] C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@usbalbin
Copy link
Contributor

usbalbin commented Dec 16, 2020

Feature gate: #![feature(array_zip)]

This is a tracking issue for the zip method on arrays.

Just like Iterator::zip combines two iterators into one, [T; N]::zip() returns a new array where every element is a tuple where the first element comes from the first array, and the second element comes from the second array. In other words, it zips two arrays together, into a single one.

let x = [1, 2, 3];
let y = [4, 5, 6];
let z = x.zip(y);
assert_eq!(z, [(1, 4), (2, 5), (3, 6)]);

Public API

pub fn zip<U>(self, rhs: [U; N]) -> [(T, U); N]

Steps / History

Unresolved Questions

@cramertj comment

[...] This seems like a pretty niche feature to me personally, but I can understand that there are situations when you might want it. I'm not sure if the libs team has a standard for "fill in the missing methods" PRs like this.

@yoshuawuyts comment

overlap with potential std::iter::Zip trait [...]

@m-ou-se m-ou-se added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 16, 2020
@leonardo-m
Copy link

Could you show two use cases?

@usbalbin
Copy link
Contributor Author

Could you show two use cases?

What made me open the PR was for doing element wise operations on vector-like objects without the need for any heap allocations or unnecessary zeroing.

struct Vector([T; N]);

impl Add<Vector> for Vector {
    fn add(self, rhs: Vector) -> Vector {
        self.0.zip(rhs.0).map(|(lhs, rhs)| lhs + rhs)
    }
}

As for a second use case, well I will have to think about that one...

Maybe bit far-fetched but here goes:

const RES: usize = WIDTH * HEIGHT;
fn compute_pixels_for_screen_on_embedded_thing(
    background: [ImgColor; RES],
    object: [ImgColor; RES]
) -> [ScreenColor; RES] {
    background.zip(object).map(magic_color_mix)
}

(which I suppose is pretty much the same use case but with different names)

@cauebs
Copy link
Contributor

cauebs commented Dec 21, 2020

Wouldn't this become obsolete after #65798?

@CryZe
Copy link
Contributor

CryZe commented Dec 21, 2020

No because iterators don't encode the length in the type, so if you intend to go back to an array you'd have to unwrap.

@leonardo-m
Copy link

No because iterators don't encode the length in the type,

Can't we give arrays a different iterator that encodes the length too? :-)

@usbalbin
Copy link
Contributor Author

No because iterators don't encode the length in the type,

Can't we give arrays a different iterator that encodes the length too? :-)

Wouldn't that be problematic in combination with methods like filter or skip etc?

@leonardo-m
Copy link

leonardo-m commented Dec 22, 2020

Yeah. We can't have a "filter" there. So only some iterators are possible. But "skip" is doable as long as the number of items to skip is given as a const generics value (and in my code the argument of "skip" is often a value known at compile-time).

@usbalbin
Copy link
Contributor Author

usbalbin commented Jan 2, 2021

No because iterators don't encode the length in the type,

Can't we give arrays a different iterator that encodes the length too? :-)

Would that be/work with #80470 or something completely different like new trait? Just asking since it seems like array IntoIter might be getting stabilized

@m-ou-se m-ou-se added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jan 6, 2021
@usbalbin
Copy link
Contributor Author

usbalbin commented Mar 1, 2021

@leonardo-m

Yeah. We can't have a "filter" there. So only some iterators are possible. But "skip" is doable as long as the number of items to skip is given as a const generics value (and in my code the argument of "skip" is often a value known at compile-time).

I have started doing some experimenting here. Atleast to me this feels like a much more reusable and flexible solution than adding all of those of methods directly to [T; N].

@MDoerner
Copy link

MDoerner commented May 6, 2021

As somebody who has stumbled upon this unstable method while implementing some const generic vector operations, I would like to point out something regarding the presented use case. To actually efficiently lift traits like Add as component-wise operations, one would rather need a zip_with operation combining zip and map, maybe with some other name more consistent with rust conventions. Using the unstable array_map from #75243 on array_zip would potentially write the array twice, at least given the current implementation.

@usbalbin
Copy link
Contributor Author

usbalbin commented May 6, 2021

@MDoerner any thoughts on something like iter_fixed?

Edit:

There is an example here for vector addition

let x = [1, 2, 3];
let y = [4, 5, 6];

let z: [_; 3] = x
   .into_iter_fixed()
   .zip(y)
   .map(|(a, b)| a + b)
   .collect();

@linclelinkpart5
Copy link

linclelinkpart5 commented Jun 20, 2021

This method has ended up being incredibly useful in my fixed-array audio DSP library, mainly in frame addition/multiplication. It allows me to do an element-wise binary operation without needing to use iterators (non-const? pah!) or having to create an incrementally-updated output array. The latter is especially nice in cases where the output type is expensive to instantiate: having to create N instances for a temporary output buffer that are immediately going to be overwritten is wasteful.

Something I'd love to suggest would be a corresponding unzip method, the inverse operation of zip. I imagine its signature looking like something along the lines of:

impl<T, U, const N: usize> [(T, U); N] {
    fn unzip(self) -> ([T; N], [U; N]) {
        // impl
    }
}

I'd be happy to work on an impl of unzip if others think it would be a useful method. I'd personally find quite a few good spots to use this in that DSP library.

@HKalbasi
Copy link
Member

@usbalbin can we get rid of into_iter_fixed and make into_iter return a fixed iterator (which is an iterator as well)?

@usbalbin
Copy link
Contributor Author

usbalbin commented Sep 10, 2021

The problem is that regular Iterators shrink every time you call next() on them (unless they are infinite). So as far as I can tell, that makes it problematic (impossible?) to make a useful type that is both Iterator and IteratorFixed(I would be happy to be proven wrong :) ).

Note however that IteratorFixed does implement IntoIterator.

@HKalbasi
Copy link
Member

hmm, yes it looks impossible. I was dreaming of an iterator with a Option<usize>, which next and similar methods are only for None (unknown sizes) but map and zip and ... can handle sized ones as well as unknowns. But it still needs two methods, one with None size for using next, and one with known size (or it can be generic but it will need explicit type annonations which is worse)

@vadixidav
Copy link
Contributor

vadixidav commented Sep 10, 2021

The problem is that regular Iterators shrink every time you call next() on them (unless they are infinite). So as far as I can tell, that makes it problematic (impossible?) to make a useful type that is both Iterator and IteratorFixed(I would be happy to be proven wrong :) ).

Note however that IteratorFixed does implement IntoIterator.

I think such a fixed iterator would have to implement a separate iterator trait, but that trait could have a method that converts it into a normal iterator. For instance, the fixed iterator could have methods that operate on it that transform it from one fixed length to another, and it could lazily evaluate to an array (no next() at all). However, the fixed iterator could have a method that converts it into a regular iterator. For instance:

// Note that an array would implement the fixed iterator trait (output equals the array itself).
let arr = [1, 2, 3];
// You could use the map method to change the array values.
let mapped = arr.map(|v| v*2);
// Then you could either turn the mapped fixed iterator into an array
let double_arr: [i32; 3] = mapped.array();
// or you could turn it into an iterator (note that the iterator values are of type `i32`, not `&i32`).
let sum: i32 = mapped.iter().sum();
// Note that it was used a second time here because the mapped array implements `Copy`.

@leonardo-m
Copy link

// Then you could either turn the mapped fixed iterator into an array
let double_arr: [i32; 3] = mapped.array();

Now that method needs to be named something like "lazy_map()"

@vadixidav
Copy link
Contributor

@leonardo-m Yes, I suppose it is too late to create a lazy array trait with a map() method at this point.

@the8472
Copy link
Member

the8472 commented Oct 26, 2022

In #103555 .zip().map() has been reported to be slow. #80094 (comment) mentions that kind of pattern as motivation.

Does anyone actually need the [(T, U); N] output? Or would a [T; N]::map_pairwise(other: [U; N]) -> [V; N] (or the iter_fixed crate) cover most needs?

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Nov 15, 2022

Potential concern: overlap with potential std::iter::Zip trait

I'm currently working on the futures-concurrency library as part of the Async WG. We're currently adding array::zip API which conflicts with the proposed API here. The API we're working on would work as a counterpart to our Merge trait, but providing a different output ordering:

//! Intended use if `Zip` were part of the stdlib.

use std::iter::{self, Zip};

let s1 = iter::repeat(0).take(5);
let s2 = iter::repeat(1).take(5);
let s3 = iter::repeat(2).take(5);

// zips N iterators together.
for [n1, n2, n3] in [s1, s2, s3].zip() {
    assert_eq!([n1, n2, n3], [0, 1, 2]);
}

// it works on tuples too!
for (n1, n2, n3) in (s1, s2, s3).zip() {
    assert_eq!((n1, n2, n3), (0, 1, 2));
}

In particular because I believe the plan is still to e.g. enable collect to work for arrays - which means what's provided by this API may be achievable through other means as well. In comparison: being able to zip arrays, tuples, and vecs through a shared API may be a lot harder, if not impossible to achieve through other means.

Conclusion

I'm not trying to say that just because we're experimenting with something, this API should be abandoned. But I'm raising this so that when a discussing pops up about potentially stabilizing this API, the fact that it would shut out (parts of 1) the other design should be weighed into consideration.

Footnotes

  1. It would certainly be possible to implement Zip just on tuples and vecs, but tbh that would be asymmetrical with how we're looking at implementing Merge and potentially Chain as well. array::zip as defined in this unstable tracking issue is the only API which would present a conflict, hence I'm raising this issue here.

@usbalbin

This comment was marked as off-topic.

@the8472
Copy link
Member

the8472 commented Mar 11, 2023

So, I'll repeat my previous question:

Is there anyone who's in favor of keeping zip as it is today or can we rip it out and then consider replacements separately?

@usbalbin
Copy link
Contributor Author

usbalbin commented Mar 11, 2023

@the8472 see my earlier comment, do you have any thoughts on that?

Edit: Just saw you mentioned it earlier.

@the8472 the8472 added the I-libs-nominated Nominated for discussion during a libs team meeting. label Mar 13, 2023
@the8472
Copy link
Member

the8472 commented Mar 13, 2023

Nominating this for removal since [(T, U); N] optimizes poorly and seems to be rarely what's really wanted.

@the8472 the8472 added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed I-libs-nominated Nominated for discussion during a libs team meeting. labels Mar 14, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Mar 14, 2023

@rfcbot close

@rfcbot
Copy link

rfcbot commented Mar 14, 2023

Team member @m-ou-se has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Mar 14, 2023
@scottmcm
Copy link
Member

scottmcm commented Mar 14, 2023

FWIW, this now at least codegens decently well for basic cases. For example, x.zip(y).map(|(x, y)| x - y) gives a simd subtraction, with no extra ceremony https://rust.godbolt.org/z/63fEGnKd6:

pub fn short_integer_zip_map(x: [u32; 8], y: [u32; 8]) -> [u32; 8] {
// CHECK: %[[A:.+]] = load <8 x i32>
// CHECK: %[[B:.+]] = load <8 x i32>
// CHECK: sub <8 x i32> %[[A]], %[[B]]
// CHECK: store <8 x i32>
x.zip(y).map(|(x, y)| x - y)
}

But I do also think that a zip_with is probably what I'd always actually want here, because eagerly getting tuples isn't particularly useful.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 28, 2023
@rfcbot
Copy link

rfcbot commented Mar 28, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@the8472 the8472 added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 28, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 7, 2023
@rfcbot
Copy link

rfcbot commented Apr 7, 2023

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 7, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 13, 2023
@bors bors closed this as completed in 88160ab May 31, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Aug 24, 2023
Remove array_zip

`[T; N]::zip` is "eager" but most zips are mapped. This causes poor optimization in generated code. This is a fundamental design issue and "zip" is "prime real estate" in terms of function names, so let's free it up again.

- FCP concluded in rust-lang/rust#80094 (comment)
- Closes rust-lang/rust#80094
- Closes rust-lang/rust#103555

Could use review to make sure we aren't losing any essential codegen tests.
r? `@scottmcm`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: [T; N] C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.