tomstewart89 / BasicLinearAlgebra

A library for using matrices and linear algebra on Arduino
MIT License
185 stars 38 forks source link

issue with matrix inversion #43

Closed JM-FRANCE closed 2 years ago

JM-FRANCE commented 2 years ago

Line 55 of NotSoBasicLinearAlgebra.h Go to file

        for (int j = 0; j <= dim; ++j)

the <= is wrong, it should be <

Also I'm not sure why you say the matrix is singular only of all the elements are null. You would need to check the determinant to see if the matrix is singular.

code to demonstrate the issue:

#include <BasicLinearAlgebra.h>
using namespace BLA;

void printMatrix(Matrix<4, 4, Array<4, 4, int>>& m) {
  for (size_t i = 0; i < m.Rows; i++) {
    for (size_t j = 0; j < m.Cols; j++) {
      Serial.print(m(i, j));
      Serial.write('\t');
    }
    Serial.println();
  }
  Serial.println();
}

void setup() {
  Serial.begin(115200); Serial.println();

  Matrix<4, 4, Array<4, 4, int>> baseMatrix;
  Matrix<4, 4, Array<4, 4, int>> inverseMatrix;
  Matrix<4, 4, Array<4, 4, int>> verif;

  baseMatrix = {1, 0, 0, 0, 0, 1, 0, 0, 1, 1, 0, 0, 0, 0, 0, 1};

  Serial.println("attempting to inverse");
  printMatrix(baseMatrix);

  int d  = Determinant(baseMatrix);
  Serial.print("Determinant = ");  Serial.println(d);

  if (Invert(baseMatrix, inverseMatrix)) {
    Serial.println("Matrix could be inveresed");
    printMatrix(inverseMatrix);

    verif = baseMatrix * inverseMatrix;
    Serial.println("Multiplying the matrix gives");
    printMatrix(inverseMatrix);
  } else {
    Serial.println("Matrix could not be inveresed");
  }
}

void loop() {}
tomstewart89 commented 2 years ago

Hey @JM-FRANCE, thanks for the info, you're quite right. I've pushed a fix it's included in v3.4.

Regarding your second point:

Also I'm not sure why you say the matrix is singular only of all the elements are null.

We actually only mark the matrix as singular if there exists a row whose elements are all zero. In that case the matrix would be rank deficient and would have a determinant of zero.

Thanks again for the info, feel free to reopen this issue if you have any further problems

JM-FRANCE commented 2 years ago

Hi

OK - yes indeed you check for a null row but that’s not necessary (just sufficient) to have a singular matrix. In order to return a correct status you would need to calculate the determinant - or update the documentation on this function to make it the responsibility of the caller to check this separately

tomstewart89 commented 2 years ago

Ah ok I see your point, indeed having a null row doesn't cover all singular matrices, thanks for the clarification.

I just pushed a small change (https://github.com/tomstewart89/BasicLinearAlgebra/commit/1c63223d809f6392959970eabdc0410e5247b647) which should correct for this. Basically in addition to the previous check we now also flag the matrix as singular if any of the diagonal elements in the LU decomposition are zero. This should be sufficient to detect singular matrices since the determinant of a matrix is the product of those diagonal elements.

As a result the Invert function now also correctly returns false for singular matrices so thanks again for the info.

JM-FRANCE commented 2 years ago

Sounds good !