ndarray icon indicating copy to clipboard operation
ndarray copied to clipboard

Feature suggestion: impl Default for Zip, and add Zip::with_shape() constructor

Open ttencate opened this issue 3 years ago • 1 comments
trafficstars

Motivating example: I'm computing 3D positions on a unit sphere from a uniform grid of (longitude, latitude) pairs. This is what it looks like now:

        const SHAPE: (usize, usize) = (HEIGHT, WIDTH);
        let cell_size_lon = TAU / WIDTH as f32;
        let cell_size_lat = PI / HEIGHT as f32;
        let lon = Array1::linspace(cell_size_lon, TAU - cell_size_lon, WIDTH)
            .into_shape((1, WIDTH)).unwrap();
        let lat = Array1::linspace(-FRAC_PI_2 + cell_size_lat, FRAC_PI_2 - cell_size_lat, HEIGHT)
            .into_shape((HEIGHT, 1)).unwrap();
        let pos = Zip::from(lat.mapv(f32::cos).broadcast(SHAPE).unwrap())
            .and(lat.mapv(f32::sin).broadcast(SHAPE).unwrap())
            .and(lon.mapv(f32::cos).broadcast(SHAPE).unwrap())
            .and(lon.mapv(f32::sin).broadcast(SHAPE).unwrap())
            .par_map_collect(|&cos_lat, &sin_lat, &cos_lon, &sin_lon| {
                Vec3::new(cos_lat * sin_lon, sin_lat, cos_lat * cos_lon)
            });

I know about azip! and par_azip! but I don't think the small reduction in character count outweighs the additional "magic" syntax, so I don't use them.

Notice the distinction between Zip::from and Zip::and even though these both serve the same purpose: add another producer to the Zip.

This lopsided distinction could be eliminated if Zip implemented Default (and/or had the customary zero-args new() constructor):

        let pos = Zip::default()
            .and(lat.mapv(f32::cos).broadcast(SHAPE).unwrap())
            .and(lat.mapv(f32::sin).broadcast(SHAPE).unwrap())
            .and(lon.mapv(f32::cos).broadcast(SHAPE).unwrap())
            .and(lon.mapv(f32::sin).broadcast(SHAPE).unwrap())
            .par_map_collect(|&cos_lat, &sin_lat, &cos_lon, &sin_lon| {
                Vec3::new(cos_lat * sin_lon, sin_lat, cos_lat * cos_lon)
            });

Even nicer (for this use case) would be if we could specify the shape once up front in the constructor, so we could use and_broadcast on all producers (not just the 2nd-4th):

        let pos = Zip::with_shape(SHAPE)
            .and_broadcast(lat.mapv(f32::cos))
            .and_broadcast(lat.mapv(f32::sin))
            .and_broadcast(lon.mapv(f32::cos))
            .and_broadcast(lon.mapv(f32::sin))
            .par_map_collect(|&cos_lat, &sin_lat, &cos_lon, &sin_lon| {
                Vec3::new(cos_lat * sin_lon, sin_lat, cos_lat * cos_lon)
            });

I can't tell whether this is at all feasible or desirable, just throwing it out there for your consideration, and so that other users with similar needs can chime in.

ttencate avatar Nov 02 '22 10:11 ttencate

with_shape works with the current model, default does maybe not. Currently Zip uses the Rust idea of an object being in a known good state as soon as it is constructed, and that's why it requires to see the first producer on construction - to set the shape.

But since we change type every time we .and something on, maybe neither idea is impossible, they are both reasonable.

bluss avatar Nov 12 '22 11:11 bluss