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
Add cumulative sum tensor operation #1722
base: main
Are you sure you want to change the base?
Conversation
Hi @allenqm
I'm willing to write that kernel in the JIT intermediate representation if you want, so the operation becomes available soon; then we can optimize it later and with the upcoming language. |
@louisfd Thanks so much for the guidance. I will remove _dim suffix. Thanks for offering to step in and write the kernel in the JIT intermediate representation. I'll take you up on that. I'm going to try and get the tch, candle, ndarray, and autodiff implementations done by EoD tomorrow. Just to be clear: I haven't written anything specific for cumprod yet. I was proposing that if we implement cumsum, then cumprod will be more straightforward as it could be described without new backend implementations (with the exception of autodiff), using the existing implementations of cumsum, exp, and log. Let me know if my assessment here seems off. |
tensor: NdArrayTensor<E, D>, | ||
dim: usize, | ||
) -> NdArrayTensor<E, D> { | ||
let mut array = tensor.array.clone().into_owned(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the underlying array struct of tensor
needs to be cloned, since NdArray's method for accumulating elements along an axis modifies an array's data inplace. Referring to this method
tch, candle, ndarray, autodiff + tests, and tensor tests have been added. Going to work on the onnx section of the contributor book next. no action needed, just fyi @louisfd |
Pull Request Template
Starting a draft PR to align on a few things with maintainers as I dive into this.
Context: Per this convo, I wanted to add a cumulative product operation to burn.
My plan is to start with a cumulative sum operation. Then cumulative product can be developed using cumulative sum, log, and exp.
@nathanielsimard, Items to align on upfront:
cumsum_dim
.cumsum
aligns with the pytorch api. In burn, operations that take an explicit dim argument seem to have a_dim
suffix. Alternatively we could remove the suffix.Checklist
run-checks all
script has been executed.Related Issues/PRs
Provide links to relevant issues and dependent PRs.
Changes
Summarize the problem being addressed and your solution.
Testing
Describe how these changes have been tested.