added ndarray class #15
Conversation
…me classic numpy's arrays capabilities
|
❤️ This is great Matthias, thank you so much! Give me a couple of days to review it and I'll merge it. |
|
I've added a few more stuffs. I keep adding elements while coding gradient descent, since I keep finding useful functions/features. |
There was a problem hiding this comment.
Thanks again for the PR Matthias! Could you address some of the things I commented here? Sorry for the delay in getting back with this, I've been trying get Travis to run the tests inside the Docker container but it's taking me a bit longer than expected.
While you are at this I'll go on trying to make that work so we have a decent CI to cover our backs. Then I'll merge this in whatever state it is by that time, it's a great addition to Crystalla as-is.
| require "./spec_helper" | ||
| require "benchmark" | ||
|
|
||
| describe Ndarray do |
There was a problem hiding this comment.
Just a stupid detail, but I'd prefer to name it NdArray instead of Ndarray
| m[1, 1].should eq(4) | ||
| end | ||
|
|
||
| it "creates a Matrix from given rows" do |
| m[0, 1].should eq(0) | ||
| end | ||
|
|
||
| it "creates a matrix of ones" do |
| it "creates a row vector given an array" do | ||
| m = Ndarray.new [3, 2, 1] | ||
| m.shape.should eq({0, 3}) | ||
|
|
There was a problem hiding this comment.
Looks like we could add an assertion accessing the row's elements here ([m[0], m[1], m[2]].should eq([3, 2, 1]))
What do you think?
| end | ||
| end | ||
|
|
||
| # context "invert" do |
There was a problem hiding this comment.
TODO (for myself): open an issue to implement invert after merging this
| @@ -37,12 +37,12 @@ module Crystalla | |||
| Matrix.new values.map(&.-), @number_of_rows, @number_of_cols | |||
| end | |||
|
|
|||
| def *(other : self) : Matrix | |||
| def * (other : self) : Matrix | |||
There was a problem hiding this comment.
I think this space slipped here
| module Crystalla | ||
|
|
||
|
|
||
| # class Dtype(T) |
There was a problem hiding this comment.
Let's remove these comments unless you have a very good reason to keep them here!
| end | ||
|
|
||
| def set_shape(values) | ||
| if values.is_a?(Array(Int32)) ||values.is_a?(Array(Float64)) |
There was a problem hiding this comment.
There are a couple of formatting issues in the PR, have you run the formatter before pushing? (crystal tool format from the root of your working copy).
There was a problem hiding this comment.
I didn't know about this tool!
And regarding this exact formatting issue, if I remember well, I ran into a weird error. I couldn't add a space after || as I could in most languages. But I'll double check!
There was a problem hiding this comment.
Weird. Did you get a compiler error or was it a syntax highlighting issue? It's compiling and passing specs with the space for me. If it's a syntax highlighting issue, it's probably due to an underfeatured editor plugin. Unsurprisingly, initial Crystal support for most editors is based on Ruby grammar.
| end | ||
| end | ||
|
|
||
| def broadcast(number) : Ndarray |
There was a problem hiding this comment.
Just out of curiosity: did you have any reason to annotate all these return types or was it just that you ported them over from the Matrix implementation? I think annotating all return types in Matrix was a bad idea, because in Crystal that makes us miss some opportunities to take advantage of polymorphism. That was an insight after coding in Crystal for a while after this :).
Either way, it's ok for now. I guess later on I'll compare this with the Matrix implementations and try to extract common code to a module.
| end | ||
|
|
||
| # reshape array C style or Fortran style | ||
| def reshape(number_of_rows, number_of_cols, order="F") |
There was a problem hiding this comment.
I think it's better if we default to order="C". Numpy does it like that, and most of the modern developers will be a bit alienated if they call reshape(x, y) and it's backwards.
|
As regards:
Big 👍 from my side |
|
Sounds good, I'll fix those over the weekend 👍 |
So this is were I am. A Ndarray class, supporting most of the methods you defined for matrices when the array is 2D (there are a few that still need to be added), and basic numpy's methods for ndarrays (reshape, broadcasting, sum, bool comparison etc). As you advised, I use column major order.
There are some differences with the current Matrix class (shape, dot product instead of *) since I've tried to stick with numpy's behaviour most of the time.
Works fine with 1D and 2D (see specs), not yet with more dimensions.
I've been able to replicate the numpy part of this course intro using this ndarray class.
Let me know what you think, what should be improved, if you have any design advices etc !