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

Support for slice assign #1351

Closed
louisfd opened this issue Nov 20, 2023 · 7 comments
Closed

Support for slice assign #1351

louisfd opened this issue Nov 20, 2023 · 7 comments

Comments

@louisfd
Copy link

louisfd commented Nov 20, 2023

I'm bringing back to life issue #663
The proposed solution of slice-scatter in #927 does not seem to be enough.
I've tried turning things around but I don't succeed in accomplishing the following:

let a =  Tensor::new([[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]]);
let b = Tensor::new( [[88.0, 99.0]]);
let new_a = a.slice_assign(b, [1..2, 1..3])
assert_eq!(new_a, [[0.0, 1.0, 2.0], [3.0, 88.0, 99.0]])

I think I'm not able to achieve this with slice_scatter because it only takes into account one dimension at a time.

@EricLBuehler
Copy link
Member

Just saw this, but relates to #1279.

@LaurentMazare
Copy link
Collaborator

let a =  Tensor::new([[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]]);
let b = Tensor::new( [[88.0, 99.0]]);
let new_a = a.slice_assign(b, [1..2, 1..3])
assert_eq!(a, [[0.0, 1.0, 2.0], [3.0, 88.0, 99.0]])

Thanks for bringing this up. Just to be sure, should the assert_eq in the example be about new_a or about a? Tensors are immutable by design in candle so it's unlikely that we will get the snippet you gave to work on a (we have some thoughts about providing in-place operations for variables rather than tensors but would really prefer avoiding it).
If it's about new_a it's probably easier to have but it would be good to have some example of models using this to see if it isn't easy to get some workaround there? (it seems like a bit of a convoluted operation so may be easier if we can avoid it and use simpler ops instead but that really depends on what you're trying to achieve here)

@louisfd
Copy link
Author

louisfd commented Nov 21, 2023

Hi Laurent,
Of course I meant new_a, sorry for the typo. I fixed it now.
In Burn we use slice_assign for instance in autoregressive attention and LSTM.
But even more important is that it's in the backward pass of our equivalent of Candle's narrow operator.

@EricLBuehler
Copy link
Member

In Burn we use slice_assign for instance in autoregressive attention and LSTM.

candle-sampling needs this slice_assign feature for the implementation of Beam search.

@LaurentMazare
Copy link
Collaborator

Hi Laurent, Of course I meant new_a, sorry for the typo. I fixed it now. In Burn we use slice_assign for instance in autoregressive attention and LSTM. But even more important is that it's in the backward pass of our equivalent of Candle's narrow operator.

Ok, I've merged #1377 that adds some pretty naive implementation for slice_assign, it's quite inefficient but avoids introducing some "base op". If this ends up being a bottleneck for performance, we could consider making something more efficient (but I kind of doubt that this will end up being in the performance critical op).

@louisfd
Copy link
Author

louisfd commented Nov 26, 2023

Thanks a lot, it works fine!

@LaurentMazare
Copy link
Collaborator

Closing again as hopefully all good now :)

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

No branches or pull requests

3 participants