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

Matrix Ruby Wrappers #56

Open
wants to merge 30 commits into
base: master
from

Conversation

@rajithv
Copy link
Contributor

commented Jun 21, 2016

No description provided.

VALUE cmatrix_dense_alloc(VALUE klass)
{
CDenseMatrix *mat_ptr = dense_matrix_new();
return Data_Wrap_Struct(klass, NULL, cmatrix_dense_free, mat_ptr);

This comment has been minimized.

Copy link
@abinashmeher999

abinashmeher999 Jun 23, 2016

Contributor

What does klass stand for here?

This comment has been minimized.

Copy link
@rajithv

rajithv Jun 23, 2016

Author Contributor

I was writing it by taking the example of cbasic_alloc. So assumed, the klass is something which is called when allocating the space bound defined in rb_define_alloc_func(c_dense_matrix, cmatrix_dense_alloc);

Shoud I remove it?

This comment has been minimized.

VALUE operand = rb_ary_shift(args);
char *s = rb_obj_classname(operand);

if(strcmp(s, "SymEngine::DenseMatrix") == 0) {

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 23, 2016

Member

Format with clang-format

This comment has been minimized.

Copy link
@rajithv

rajithv Jun 23, 2016

Author Contributor

Will format all code with clang-format. Please disregard formating issues for now.

@rajithv

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2016

@abinashmeher999 @isuruf

Please check the following:

irb(main):003:0> A = SymEngine::DenseMatrix.new(arr)
=> #<SymEngine::DenseMatrix([1, 2]
[3, 4]
[5, 6]
)>
irb(main):004:0> A.to_s
=> "[1, 2]\n[3, 4]\n[5, 6]\n"

Also, does it need row count and column count?

The to_s output is not nice.. right? What are our options?

@zverok

This comment has been minimized.

Copy link
Collaborator

commented Jun 23, 2016

There are two main things to consider:

  1. inspect: it should be as informative, as possible, but with reasonable limitation; for example, what we did in daru:
v = Daru::DataFrame.new(a: [1,2,3], b: [4,5,6])
# => #<Daru::DataFrame(3x2)>
#       a   b
#   0   1   4
#   1   2   5
#   2   3   6 

v = Daru::DataFrame.new(a: (1..100), b: (201..300))
# => #<Daru::DataFrame(100x2)>
#       a   b
#   0   1 201
#   1   2 202
#   2   3 203
#   3   4 204
#   4   5 205
#   5   6 206
#   6   7 207
#   7   8 208
#   8   9 209
#   9  10 210
#  10  11 211
#  11  12 212
#  12  13 213
#  13  14 214
#  14  15 215
# ... ... ... 

# ^ above is full output of #inspect of large DataFrame, including those "..."
  1. to_s: for "non-stringy" types it should be as short as possible (while saving some informativeness), consider it used in contexts like raise ArgumentError, "Expected blah, yet received #{wtf}" ← this uses #to_s to render some error message... And somebody would output it.
rajithv added 2 commits Jun 23, 2016
@rajithv

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2016

@zverok Thanks. I'll come up with a scheme considering the points you mentioned. I have a better idea now.

@isuruf mentioned that to_s is going to change in C++ layer soon, so I'll wait for that to happen. Until then, I'll show the number of rows and number of columns in inspect.

@rajithv rajithv force-pushed the rajithv:matrix branch from bc6c130 to 5c739d1 Jul 4, 2016


} else if (argc == 2) {

// SymEngine::DenseMatrix(no_rows, no_cols)

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 6, 2016

Member

This is needed in C++ layer, but not needed in Ruby right?

This comment has been minimized.

Copy link
@rajithv

rajithv Jul 7, 2016

Author Contributor

@isuruf should I remove it?

I suppose this can be used to generate an empty Matrix of size RxC, where individual elements can be assigned with set. But again, there's no point in doing that.

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 7, 2016

Member

Yes, for that use case, it's better to use zeros

@rajithv rajithv changed the title [WIP] Matrix Ruby Wrappers Matrix Ruby Wrappers Jul 7, 2016

@rajithv

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

@isuruf @abinashmeher999 @zverok ready for review.

@zverok I'm not too sure about the specs. Please advice if I'm not doing it right :)

@zverok

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2016

Looking at it!

@@ -0,0 +1,7 @@
module SymEngine
class MatrixBase

This comment has been minimized.

Copy link
@zverok

zverok Jul 7, 2016

Collaborator

Shouldn't it say class DenseMatrix here?

module SymEngine
class MatrixBase
def inspect
"#<#{self.class}()>"

This comment has been minimized.

Copy link
@zverok

zverok Jul 7, 2016

Collaborator

I assume both dense and sparse matrices should have the same inspect "#<#{self.class}(#{rows}x#{cols})>", so you can define it here in MatrixBase only. Or am I missing something?

This comment has been minimized.

Copy link
@rajithv

rajithv Jul 7, 2016

Author Contributor

Actually the sparse matrix is almost non-existent. The C++ code is still being developed. So, it's only possible to have an empty SparseMatirx, or have one, and then manually enter elements using set. This makes the inspect giving errors which comes from __str__ method in C++ code.

That's why I have a more descriptive one for DenseMatrix separately.

This comment has been minimized.

Copy link
@zverok

zverok Jul 7, 2016

Collaborator

OK. Let's just not forget to recheck (and possibly join) the code when SparseMatrix will come to live.

This comment has been minimized.

Copy link
@rajithv

rajithv Jul 7, 2016

Author Contributor

I will add an issue so that I wouldn't forget it!


end

describe 'more dense_matrix functions' do

This comment has been minimized.

Copy link
@zverok

zverok Jul 7, 2016

Collaborator

Not a really descriptive describe description ;)

let(:matA) { SymEngine([[3, 1, 2],[5, 7, 5], [1, 2, 3]]) }
let(:b) { SymEngine([[4],[4],[4]]) }

it do

This comment has been minimized.

Copy link
@zverok

zverok Jul 7, 2016

Collaborator

Only one-line its are OK to leave without descriptions, multi-line ones should be like it 'is blah blah' do (and when you can't invent good description, it could be a sign of wrong examples grouping).

module SymEngine
class MatrixBase
def inspect
"#<#{self.class}(#{rows}x#{cols})>"

This comment has been minimized.

Copy link
@zverok

zverok Jul 7, 2016

Collaborator

Please add specs for this inspect. It is quick and easy to write, and will save you from errors like inspecting into something like #<DenseMatrix(5x> or NoMethodErrors or whatnot.

@zverok

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2016

I've left some comments.
Also, I am a bit concerned about the fact that only "happy path" is tested. What if I'll try to multiply DenseMatrix by string or nil? Would there be an informative exception? Would there be at least some exception (and not some nasty crash in the middle of C code). Testing behavior on failures and corner cases (for ex., behavior of empty matrices) could be tiresome, but without it we can't be sure code is actually working.

@rajithv rajithv force-pushed the rajithv:matrix branch from 54d0685 to e447e97 Jul 7, 2016

@@ -66,35 +66,6 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"or create a variable"

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 8, 2016

Member

Can you remove these changes from this PR?

@@ -83,7 +83,7 @@ void sympify(VALUE operand2, basic_struct *cbasic_operand2)
{
VALUE ret = check_sympify(operand2, cbasic_operand2);
if (ret == Qfalse) {
rb_raise(rb_eTypeError, "%s can't be coerced into SymEngine::Basic",
rb_raise(rb_eTypeError, "%s can't be coerced into a SymEngine type.",

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 8, 2016

Member

Why this change?

This comment has been minimized.

Copy link
@rajithv

rajithv Jul 8, 2016

Author Contributor

reverting it.

I changed that when I thought I was going to incorporate sympifying arrays into matrices into this. Have forgotten to change it back.

@rajithv rajithv force-pushed the rajithv:matrix branch from 529da55 to 61cee1a Jul 8, 2016

VALUE operand = rb_ary_shift(args);
char *s = rb_obj_classname(operand);

if (strcmp(s, "SymEngine::DenseMatrix") == 0) {

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 8, 2016

Member

You can move this if else function to a new function, sympify_matrix

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 8, 2016

Member

You can use a C API call to check that operand is a DenseMatrix instead of string comparisons

// SymEngine::DenseMatrix(NMatrix)
rb_raise(rb_eTypeError, "TODO");

} else if (strcmp(s, "Array") == 0) {

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 8, 2016

Member

You can use rb_array_check_type to check that it is an array


} else if (argc == 2) {

// SymEngine::DenseMatrix(no_rows, no_cols)

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 8, 2016

Member

Remove this constructor as it is not needed


char *s = rb_obj_classname(operand);

if (strcmp(s, "SymEngine::DenseMatrix") == 0) {

This comment has been minimized.

Copy link
@isuruf

isuruf Jul 8, 2016

Member

You can use the sympify_matrix function I mentioned so that a SymEngine::DenseMatrix can be equal to a Ruby array.

expect(matA.rows). to eq (3)
expect(matA.cols). to eq (3)
end
end

This comment has been minimized.

Copy link
@zverok

zverok Jul 9, 2016

Collaborator

I think, you can completely omit those describes including only one example each, its texts are pretty informative and well-written 👍
Just less cognitive load would be to read it this way:

describe SymEngine::DenseMatrix do
  describe 'convert' do
    subject { SymEngine([[1, 2],[3, 4]]) }

    it { is_expected.to be_a SymEngine::DenseMatrix }
    its(:to_s) { is_expected.to eq "[1, 2]\n[3, 4]\n" }
  end

  describe 'methods without arguments' do # that descibes accurately why you group it
    subject { SymEngine([[4, 3],[3, 2]]) }

    # moving those two here as they are pretty basic & have no arguments
    its(:rows) { is_expected.to eq (2) }
    its(:cols) { is_expected.to eq (2) }

    its(:inv) { is_expected.to be_a SymEngine::DenseMatrix }
    its(:inv) { is_expected.to eq SymEngine([[-2, 3],[3, -4]]) }

    its(:FFLU) { is_expected.to be_a SymEngine::DenseMatrix }
    its(:FFLU) { is_expected.to eq SymEngine([[4, 3],[3, -1]]) }

    its(:LU) { is_expected.to eq [SymEngine([[1, 0],[SymEngine(3)/SymEngine(4), 1]]), 
                                  SymEngine([[4, 3],[0, SymEngine(-1)/SymEngine(4)]])] }

    its(:LDL) { is_expected.to eq [SymEngine([[1, 0],[SymEngine(3)/SymEngine(4), 1]]), 
                                  SymEngine([[4, 0],[0, SymEngine(-1)/SymEngine(4)]])] }

    its(:FFLDU) { is_expected.to eq [SymEngine([[4, 0],[3, 1]]), SymEngine([[4, 0],[0, 4]]), 
                                     SymEngine([[4, 3],[0, -1]])] }

    its(:det) { is_expected.to eq (-1)}  

  end

  describe 'methods with arguments' do

    let(:mat1) { SymEngine([[1, 2],[3, 4]]) }
    let(:mat2) { SymEngine([[4, 3],[2, 1]]) }
    let(:a) { SymEngine(4) }
    let(:matA) { SymEngine([[3, 1, 2],[5, 7, 5], [1, 2, 3]]) }
    let(:b) { SymEngine([[4],[4],[4]]) }

    it 'adds and multiplies' do
      expect(mat1 + mat2).to eq(SymEngine([[5, 5],[5, 5]]))
      expect(mat1 + a).to eq(SymEngine([[5, 6],[7, 8]]))
      expect(mat1 * mat2).to eq(SymEngine([[8, 5],[20, 13]]))
      expect(mat1 * a).to eq(SymEngine([[4, 8],[12, 16]]))
    end  

    it 'performs transpose and submatrix' do
      expect(mat1.transpose).to eq(SymEngine([[1, 3],[2, 4]]))
      expect(mat1.submatrix(0, 0, 1, 0, 1, 1)).to eq(SymEngine([[1],[3]]))
    end

    it 'solves Ax = b' do
      expect(matA.LU_solve(b)).to eq(SymEngine([[SymEngine(12)/SymEngine(29)],
                                             [SymEngine(-32)/SymEngine(29)],
                                             [SymEngine(56)/SymEngine(29)]
                                             ]))  
    end

    it 'gets and sets elements' do
      expect(mat1.set(0, 0, a)).to eq(SymEngine([[4, 2],[3, 4]]))
      expect(mat1.get(1, 0)).to eq(SymEngine(3))
    end
  end
end

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.