NMatrix#sum needs NMatrix#cumsum alias
kingroryg opened this issue · 11 comments
I don't think nmatrix has a cumulative sum functionality similar to https://docs.scipy.org/doc/numpy/reference/generated/numpy.cumsum.html
or
https://in.mathworks.com/help/matlab/ref/cumsum.html .
It's a really handy tool when dealing with image matrices. I'd like to work on this if this isn't already being worked upon ?
P.S. : I'm a newbie, can I please be introduced to the appropriate sections of the codebase ?
Hi @saru95, sorry for the delayed response!
This feature does exist, but it isn't called cumsum. If you look in lib/nmatrix/math.rb, you'll see there's a function defined there called sum.
It looks like we need to add an alias to it for cumsum, which you can do as a pull request if you like. You'd just add a line right below the function's definition:
alias :cumsum :sum
I'm updating the issue subject to reflect this. Please let me know if I've got anything wrong.
I've initiated a tiny PR #556 . However, can you lead me to locations where I'd be able to edit the docs and specs regarding this.
Thanks for your help !
@saru95 The docs are auto-generated from the comments above the function. So you'll want to add a line to those docs reflecting that #cumsum can be used in lieu of #sum.
@MohawkJohn Just a quick question. How would sum handle cumulative sum for this example :
N = NMatrix
a = N[ [1,2,3], [4,5,6] ]
a.sum.to_a
# => [5, 7, 9]
b.sum(1).to_a
# => [[6], [15]] I should be obtaining [ 1, 3, 6, 10, 15, 21] ?
cc: @v0dro
AFAIK you should specify the dimension over which you want the cumsum.
I think its safe to assume that returning a 1D vector is OK. @MohawkJohn will know better, of course.
So, basically in the example I mentioned we can obtain cumulative sum along a row or a column. That is handled. However, cumsum of the entire array irrespective of its dimensions isn't handled , i.e., matrix isn't flattened before its cumsum is calculated. Should I work on that feature or is it not required at the moment ?
You could definitely work on that feature.
Thanks ! I'll push the alias and its docs for the time being.
Added a PR #564 . Sorry for the long absence. Was stuck in a hectic internship for a long time.
Fixed by #564.