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

No way this program could possibly work. #1

Open
smacl61 opened this issue Oct 18, 2023 · 4 comments
Open

No way this program could possibly work. #1

smacl61 opened this issue Oct 18, 2023 · 4 comments

Comments

@smacl61
Copy link

smacl61 commented Oct 18, 2023

You have a procedure called determinant that calls itself while in a loop. On top of that it calls make2darray over and over endlessly on entering determinant consuming alot of memory eventually leading to memory errors. Also you never release the dynamic allocation.

You can't have procedures calling itself - it's terrible practice. There's a famous programmers site named after the condition it causes - stackoverflow :)

I'd fix it for you but I don't know enough about your algo. I just need a working polyfit in c++.

@IanikPlante
Copy link
Contributor

The procedure determinant calls itself recursively, with a smaller matrix size each time. The typical example given for recursion is the calculation of a factorial number. I have not tested the code with large datasets, but for small datasets I am not aware of memory errors.

The code should work. However, I had problems with number precision in some cases, even if double precision numbers are used. If you have specific data for which the code is not working, I can have a look at it.

@leaguecn
Copy link

leaguecn commented Jun 1, 2024

Hi, guys! I have one alternative method.

// add the eigen header files
#include <Eigen/SVD>
#include <Eigen/Dense>

// replace the determinant solver as following code
double determinant(double **a, const size_t k) {

    double det = 0.;
    if (k == 1) return (a[0][0]);    

    Eigen::MatrixXd A(k, k);
    for (size_t i=0; i<k; i++){
      for (size_t j=0; j<k; j++){
        A(i, j) = a[i][j];
      }
    }
    det = A.determinant();
    return (det);
}

@SoothingMist
Copy link

When attempting to compile this code in Visual Studio, a good many errors and warnings are generated.

@jongithub11
Copy link

The determinant routine was replaced by an SVD version and works fine. When compiling under VS2010, the memory allocation for arrays need to be changed. Also, the calculatePoly function needs to have the for loop changed from i=0;i<n;i++ to i=0;i<n+1;i++. Other than those two items, it works great. Many thanks for posting the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants