[ruby-core:25624] [Bug #1532] Improved matrix.rb [patch]

Issue #1532 has been updated by Marc-Andre Lafortune.

File f_matrix_access.diff added
File a_matrix_creation.diff added
File b_matrix_empty.diff added
File c_matrix_trace.diff added
File d_matrix_cleanup.diff added
File e_matrix_enum.diff added

Here is my previous patch simplified and broken down in 6 successive patches (a-f)

*** Summary ***

The following changes are completely compatible for code that used the library in a mathematically sound manner. Potential incompatibilities would arise only for usage that does not make mathematical sense.

* API changes *

(a) Argument checking for matrix creations

Current: "although matrices should theoretically be rectangular, this is not enforced by the class" (as per documentation)

Change to: "matrices must be rectangular, otherwise an ErrDimensionMismatch is raised."

This would make the library behave in a sane manner, with error messages that are dependable and follow the established principle of detecting errors early [ruby-core:23664]

Patch attached: a_matrix_creation.diff

(b) Handle edge case Matrices

Like Mathematica, MatLab and others, the library should deal properly with matrices with a number of columns or rows of 0.

Patch attached: b_matrix_empty.diff

(c) Matrix#trace should raise an ErrDimensionMismatch if the matrix is not square

Mathematically speaking, the trace is only defined for square matrices (ref: wikipedia, mathworld)
Raising an ErrDimensionMismatch would bring #trace in line with #inverse

Patch attached: c_matrix_trace.diff

(d) Matrix#compare_by_row_vectors, Vector#compare_by and Vector#init_elements should be made private or disappear.

As per the documentation, these are not meant to be used.

Patch attached: d_matrix_cleanup.diff

* Compatible API changes *

(e) Enumerators should be returned when no blocks are given for methods like #map, etc...

This would bring the library in line with Ruby 1.8.7+ which returns enumerators in such cases

Patch attached: e_matrix_enum.diff

(f) Consistent results when accessing elements with out of bounds indices.

This would make the library in line with Ruby 1.8.7+ which returns enumerators in such cases

Patch attached: f_matrix_access.diff



*** Detailed examples of problems ***

(a) Argument checking for matrix creations

1) Plain wrong arguments:
Matrix[nil]     # => no expection is raised
Matrix[:hello]  # => no expection is raised
Matrix[4]       # => no expection is raised

Later on, confusing results or strange errors will happen. My two favorites:

Matrix[:hello].rank  # ==> NoMethodError: undefined method `quo' for "e":String

Matrix[42].transpose  # ==> Matrix[[0], [1], [0], [1]]
                      # or Matrix[[0], [1], [0], [1], [0], [1], [0], [0]]
                      # (depending on the platform)

2) Most methods will result the wrong result or raise an unintuitive error in case of uneven rows

Matrix[[1,2],[3]].rank  # ==> NoMethodError: undefined method `-' for nil:NilClass
Matrix[[0,0],[0,0]] + Matrix[[1,2], [3,4,5,6,7,8]]  # ==> does not raise error
Matrix[[1,2],[3]].square?  # ==> true

3) Array-like arguments

Moreover, basic conversion to arrays should be attempted, in particular from Vectors:

a = Matrix[[1,2],[3,4]]               # => Matrix[[1, 2], [3, 4]]
b = Matrix.rows([a.row(0), a.row(1)]) # => Matrix[Vector[1, 2], Vector[3, 4]]
a == b  # => false

The attached patch matrix_creation.diff changes the behaviour and documentation to insure that arguments passed to the Matrix creators are correct.

(related to redmine issues #1517, 1515)

(b) Handle edge case Matrices

Matrix[].row_size  # ==> 0, ok!
Matrix[].column_size # ==> raise an error, should be 0
Matrix[].determinant  # ==> raise an error, should be 1
m = Matrix[[], []]
m.transpose.transpose == m  # ==> false, should be true for any m

While an alternative would be to raise and error, the best solution is to handle empty matrices properly, both 0x0, 0xn and nx0, as does Mathematica, MatLab, GNU Octave, etc...
See doc and references to the literature in:
  http://www.gnu.org/software/octave/doc/interpreter/Empty-Matrices.html

(c) Matrix#trace should raise an ErrDimensionMismatch if the matrix is not square

Matrix[[1],[2]].trace  # ==> returns 1, which makes no sense mathematically
Matrix[[1,2]].trace   # ==> raises a NoMethodError: undefined method `[]' for nil:NilClass

(d, e) Enumerators should be returned when no blocks are given for methods like #map, etc...

No additional comment

(f) Consistent results when accessing elements with out of bounds indices.
m = Matrix[[1]]  # Example with 1x1 matrix

m[2,0]  # ==> NoMethodError: undefined method `[]' for nil:NilClass
m[0,2]  # ==> nil

m.row(2)    # ==> TypeError: can't dup NilClass
m.column(2) # ==> Vector[nil]

m.minor(2..2, 0..0) # ==> NoMethodError: undefined method `collect' for nil:NilClass
m.minor(0..0, 2..2) # ==> Matrix[nil]

Accessing elements should behave in a consistent way if either row or col is out of bounds
Moreover, Matrix[nil] is not a matrix!
  
Solutions:
1) To be consistent with array lookup using [], Matrix#[] should return nil for out of bounds elements.

2) #row, and #column should return nil, to be coherent with with Array#at, etc... and be most useful.

3) The same way Matrix#row and #col can be related to Array#at,
Matrix#minor should have similar semantics to Array#slice. If either starting point
is out of bounds, nil is returned. Otherwise a Matrix is returned, although it can
be smaller than what was requested. This is similar to
  [:a, :b].slice(3..10)  # => nil
  [:a, :b].slice(2..10)   # => []
  [:a, :b].slice(1..10)   # => [:b]
  Matrix[[1], [2]].minor(0..10, 2..10) # => nil
  Matrix[[1], [2]].minor(0..10, 1..10) # => Matrix[[], []]
  Matrix[[1], [2]].minor(1..10, 0..10) # => Matrix[[2]]

(see redmine #1518)

Cleanup, optimizations and bug fixes present in the original patch have already been committed.



*** Closing notes ***

Numerous and very basic bugs have been present for a long time (issues #1020, 1531, 2106, 2107, 2108 all fixed in upcoming versions of Ruby). This seem to indicate that very few people, if any, actually use the matrix library.

Nevertheless I believe that fixing it is important. Moreover I think we should not hesitate in modifying the API for edge cases as long as maintain compatibility for the potential few users using the library correctly.

I would be happy and honored to be the maintainer of this library.

----------------------------------------
http://redmine.ruby-lang.org/issues/show/1532

----------------------------------------
http://redmine.ruby-lang.org