SciRuby / nmatrix

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

fix spec: add NMatrix#positive_definite? for NMatrix-JRuby #594

Closed prasunanand closed 7 years ago

prasunanand commented 7 years ago

Fixes #593.

translunar commented 7 years ago

You'll have to clean this up a bit and convert it to Ruby/Java, but here's code for computing rcond:

template <int n>
double matrix_norm(const double A[][n], const int& m) 
{
  double max_norm1 = 0.0;

  for (int jj = 0; jj < n; ++jj) {

    double norm1 = 0.0;

    for (int ii = 0; ii < m; ++ii) {
      norm1 += std::abs(A[ii][jj]);
    }

    if (norm1 > max_norm1)
      max_norm1 = norm1;
  }

  return max_norm1;
}

template <int m, int n>
double rcond(const double A[][n], const double A_inv[][m]) 
{
  double a    = 1.0 / matrix_norm<n>(A, m);
  double ainv = 1.0 / matrix_norm<m>(A_inv, n);

  return a*ainv;
}

I think that's going to be both more accurate and more efficient than calculating determinants.

prasunanand commented 7 years ago

Thanks @mohawkjohn . I will do it.

translunar commented 7 years ago

@prasunanand I also don't understand — this code seems to totally duplicate the #positive_definite? code here in nmatrix/math.rb. Why can't that code be used? Why are we maintaining separate but identical code in the jruby directory?

translunar commented 7 years ago

@prasunanand Wanted to make sure you didn't miss this.

prasunanand commented 7 years ago

@mohawkjohn ,Sorry, I couldn't solve it earlier as I had my exams.

I agree that nmatrix/math.rb and nmatrix/jruby/math/rb have a lot of common code.

I will work on resolving this issue.

translunar commented 7 years ago

@prasunanand I just want to express here that this is really sloppy. It never should have made it into the NMatrix codebase. DRY (don't repeat yourself) is an important principle and, when followed, it may prevent bugs like this one.

cc @v0dro

translunar commented 7 years ago

@prasunanand Any luck?

v0dro commented 7 years ago

@prasunanand can you finish this ASAP? It was part of your objectives last year and should have been done a long time ago. Its not a lot of programming either.

prasunanand commented 7 years ago

Please check this commit: https://github.com/prasunanand/nmatrix/commit/be6517746f88ef0027bc844f17878d938b277de2

I have placed NMatrix methods common to mri and jruby in lib/nmatrix/math.rb. Methods specific to MRI have been moved to lib/matrix/cruby/math.rb. Methods specific to JRuby are in lib/nmatrix/jruby/math.rb.

There is no more duplicate code.

translunar commented 7 years ago

This looks better. Can you please add it to the current pull request (or even create a new one) so we can review it?

prasunanand commented 7 years ago

Created a new PR. https://github.com/SciRuby/nmatrix/pull/598 Please review :) .