-1

The code below calculates the determinant of a matrix of order q recursively. It works for q=3 and q=2 but for q=4 it outputs garbage values which change every time I run the program: What is going wrong here?

#include <stdio.h>
#include <math.h>

int det(int q, int arr[q][q]);
int main(void)
{
    int arr[4][4] = {
            {2,4,9,8},
            {6,3,4,5},
            {5,7,8,6},
            {3,2,5,7}
            };
    printf("value of determinant is %d\n", det(4, arr));
}

int det(int q, int arr[q][q])
{   
    if(q>2)
    {
    int i, j, k, m, n, s[q-1][q-1], d=0, cof;
    for(k=-1,i=0,j=0;k<q-1;k++)
    {
        i=0;j=0;
        for(m=1;m<q;m++,i++)
        {
            n=0;j=0;
            for(n,j;n<k+1;n++,j++)
            {
                s[i][j] = arr[m][n];
            }
            n=q-1+k;
            for(n;n<q;n++,j++)
            {
                s[i][j] = (arr[m][n]);
            }
        }
        cof = (arr[0][k+1])*(pow(-1,k+1));
        d += cof*det(q-1, s);
    }
    return d;
    }
    else if(q==2)
    {
        int d = ((arr[0][0])*(arr[1][1])-(arr[0][1])*(arr[1][0]));      
        return d;
    }
}
Stormlight
  • 161
  • 3
  • 10
  • 1
    Do you use a debugger? Have you tried stepping through with break points? Where exactly is the problem? – ryyker Oct 03 '17 at 12:53
  • Did you even try _basic_ debugging. – Jabberwocky Oct 03 '17 at 12:55
  • 1
    Why are you using recursion for this? The code was too efficient and too readable? And why do you give all your variables nonsense names with one single letter? – Lundin Oct 03 '17 at 13:26
  • @Lundin: Recursion was the first thing that came to my mind since when we take determinants we essentially sum (cofactors*another determinant) so its like calling the function again essentially. Is there a better way? – Stormlight Oct 03 '17 at 13:34
  • 1
    I don't have any special objection to the recursion -- it is a natural way to structure this algorithm. But the code presented *is* difficult to read, on account of (at least) many meaningless variable names, insufficient use of whitespace, excessive use of parentheses, and needlessly broad variable scopes. – John Bollinger Oct 03 '17 at 13:37
  • I'm doubtful that this reliably produces correct results for 3x3 matrices, much less higher-dimensional ones, though it might not manifest the same failure mode for 3x3. This looks wrong: `n=q-1+k;`. – John Bollinger Oct 03 '17 at 13:42
  • @CodeChef123 Yes, use loops. Recursion in C is not the same thing as mathematical recursion. C recursion eats lots of resources in terms of execution speed and stack usage. It is also dangerous and hard to read. And the compiler can rarely optimize it (needs to be tail recursion - this is not). Therefore recursion should be avoided like the plague in 99% of all use-cases. – Lundin Oct 03 '17 at 13:43
  • Also, do not use `pow()` here. More generally, it's a red flag if you ever think you need `math.h` in a program that (supposedly) performs only integer arithmetic. – John Bollinger Oct 03 '17 at 13:44
  • @John Bollinger: I suppose I could have used an if condition to check for parity of k but isn't that a much longer way rather than just using a library function? What's wrong in taking advantage of it? – Stormlight Oct 03 '17 at 13:52
  • @CodeChef123 `pow()` computes a *floating-point* result, which may not be exact, even when the arguments have no non-zero fraction digits. It is also far slower to call a function than to perform arithmetic. Since you're simply alternating between positive and negative, I would just use an integer variable to carry the sign -- its value will alternate between +1 and -1, you would apply it by simple (integer) multiplication, and you would flip it on each iteration. – John Bollinger Oct 03 '17 at 13:58
  • All the elements of `s` are not assigned before calling `det(q-1, s);`. Leads to UB. – chux - Reinstate Monica Oct 03 '17 at 14:05
  • Alternative to `pow(-1,k+1)` --> `(k%2 ? -1 : 1)` – chux - Reinstate Monica Oct 03 '17 at 14:17
  • What is the expected result? – chux - Reinstate Monica Oct 03 '17 at 14:18

1 Answers1

1

In your code, the main root of the problem is the algorithm.I suggest you refresh the determinant concepts.You can review the code below to check mistakes in the algorithm you used.

#include <stdio.h>
#include <math.h>

int det(int q, int arr[q][q]);
int main(void)
{
    int arr[5][5] = {
            {2,4,9,1,2},
            {6,3,4,2,6},
            {5,7,8,6,9},
            {9,1,5,3,3},
            {3,2,5,3,9}
            };
    printf("value of determinant is %d\n", det(5, arr));
    return 0;
}

int det(int q, int arr[q][q])
{
    if(q>2)
    {
        int resultOfSubStep = 0 ; //Final result that will be returned
        int smallerMatrix[q-1][q-1] ; //Matrix in which smaller matrix are to be stored which will be given as arguments to det function in current recursion 
        int i = 0 ; 
        for(;i<q;i++){
            int j = 0 ;

            for (;j<q-1;j++) { //This for loop copies the element required from arr into smallerMatrix
                int counter = 0 ;
                int k = 0 ;
                for (;k<q;k++){
                    if(k == i)continue;
                    smallerMatrix[j][counter] = arr[j+1][k];
                    counter++;
                }
            }
            int k1 = arr[0][i]*det(q-1, smallerMatrix); //This is the result of determinant of q-1*q-1 matrix
            resultOfSubStep += pow(-1,i)*k1; //multiplied by -1 or +1 according to the position of root element
        }
        return resultOfSubStep;
    }
    else if(q==2)
    {
        int d = ((arr[0][0])*(arr[1][1])-(arr[0][1])*(arr[1][0]));
        return d;
    }
    else return arr[0][0];
}

In the comments section, you asked for other methods.In my opinion, LU decomposition is the easiest one. you can check details for it on LU Decomposition.In this method, you have to reduce the given matrix to upper or lower triangular matrix and then just take the product of diagonal elements.So no need for recursion. Hope this helps.

Mandar Sadye
  • 689
  • 2
  • 9
  • 30