SciRuby / nmatrix

Dense and sparse linear algebra library for Ruby via SciRuby
Other
469 stars 133 forks source link

NMatrix#sum needs NMatrix#cumsum alias #555

Closed lprimeroo closed 6 years ago

lprimeroo commented 7 years ago

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 ?

translunar commented 7 years ago

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.

lprimeroo commented 7 years ago

I've initiated a tiny PR https://github.com/SciRuby/nmatrix/pull/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 !

translunar commented 7 years ago

@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.

lprimeroo commented 7 years ago

@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

v0dro commented 7 years ago

AFAIK you should specify the dimension over which you want the cumsum.

v0dro commented 7 years ago

I think its safe to assume that returning a 1D vector is OK. @mohawkjohn will know better, of course.

lprimeroo commented 7 years ago

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 ?

translunar commented 7 years ago

You could definitely work on that feature.

lprimeroo commented 7 years ago

Thanks ! I'll push the alias and its docs for the time being.

lprimeroo commented 7 years ago

Added a PR https://github.com/SciRuby/nmatrix/pull/564 . Sorry for the long absence. Was stuck in a hectic internship for a long time.

translunar commented 6 years ago

Fixed by #564.