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

Impl a lifetime-relaxed broadcast for ArrayView #1219

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

roblabla
Copy link

@roblabla roblabla commented Oct 10, 2022

ArrayView::broadcast has a lifetime that depends on &self instead of its internal buffer. This prevents writing some types of functions in an allocation-free way. For instance, take the numpy meshgrid function: It could be implemented like so:

fn meshgrid_2d<'a, 'b>(coords_x: ArrayView1<'a, X>, coords_y: ArrayView1<'b, X>) -> (ArrayView2<'a, X>, ArrayView2<'b, X>) {
    let x_len = coords_x.shape()[0];
    let y_len = coords_y.shape()[0];

    let coords_x_s = coords_x.into_shape((1, y_len)).unwrap();
    let coords_x_b = coords_x_s.broadcast((x_len, y_len)).unwrap();
    let coords_y_s = coords_y.into_shape((x_len, 1)).unwrap();
    let coords_y_b = coords_y_s.broadcast((x_len, y_len)).unwrap();

    (coords_x_b, coords_y_b)
}

Unfortunately, this doesn't work, because coords_x_b is bound to the lifetime of coord_x_s, instead of being bound to 'a.

This PR introduces a new function, broadcast_ref, that behaves exactly like broadcast, but the returned arrayview is bound to the lifetime of the internal storage instead.

@roblabla
Copy link
Author

The broadcast_ref implementation is copy pasted from broadcast, which is kinda bad. The common code (in particular, upcast) should probably be moved to a common module, but I'm not sure where it should go.

@jturner314
Copy link
Member

Thanks for this PR! I agree that this method would be useful.

This is related to issue #1208, which discussed slicing. However, broadcasting is different, because we can't have a broadcasting equivalent of .slice_move() for arbitrary storage types. We have to have a method specific to ArrayView, as in this PR.

I don't like the name broadcast_ref much, but it's fine if we can't come up with anything better.

The broadcast_ref implementation is copy pasted from broadcast, which is kinda bad. The common code (in particular, upcast) should probably be moved to a common module, but I'm not sure where it should go.

src/dimension/mod.rs would be a good place for upcast, since there are other similar functions in that file, e.g. do_slice. I think just moving upcast into that file would be sufficient; the rest of the code in broadcast is small enough to just duplicate, IMO.

This will allow upcast to be reused in other functions, such as the
upcoming ArrayView::broadcast_ref.
ArrayView::broadcast has a lifetime that depends on &self instead of its
internal buffer. This prevents writing some types of functions in an
allocation-free way. For instance, take the numpy `meshgrid` function:
It could be implemented like so:

```rust
fn meshgrid_2d<'a, 'b>(coords_x: ArrayView1<'a, X>, coords_y: ArrayView1<'b, X>) -> (ArrayView2<'a, X>, ArrayView2<'b, X>) {
    let x_len = coords_x.shape()[0];
    let y_len = coords_y.shape()[0];

    let coords_x_s = coords_x.into_shape((1, y_len)).unwrap();
    let coords_x_b = coords_x_s.broadcast((x_len, y_len)).unwrap();
    let coords_y_s = coords_y.into_shape((x_len, 1)).unwrap();
    let coords_y_b = coords_y_s.broadcast((x_len, y_len)).unwrap();

    (coords_x_b, coords_y_b)
}
```

Unfortunately, this doesn't work, because `coords_x_b` is bound to the
lifetime of `coord_x_s`, instead of being bound to 'a.

This commit introduces a new function, broadcast_ref, that does just
that.
@jreniel
Copy link

jreniel commented Feb 2, 2024

Hello!
I was looking for an equivalent of numpy's meshgrid on ndarray. Because I couldn't find it, I came up with my own version, which should work in n-dimensions. It is currently hosted here. I think this should belong into ndarray, and not in a separate crate, so I'd be more than happy, provided that you find my implementation satisfying, to see this code directly integrated into ndarray, instead of having a separate crate as it currently is.

@nilgoyette
Copy link
Collaborator

@jreniel Please open an issue if you want to discuss an implementation of meshgrid. It's not related to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants