Skip to main content
added 179 characters in body
Source Link
ALX23z
  • 2.5k
  • 7
  • 15
  1. Instead of #define use enum or constexpr for the constant numbers.

  2. I don't think that the number of time data or number of points should be absolute constants - rather they should be determined during runtime. Else you have to recompile your code each time you receive new data.

  3. Better declare some sort of output path instead of relying on ../, also you need to make sure that the directories exist, else it will fail to save.

  4. You don't want to store small pieces of data over huge amount of files and directories. Better make a serialization standard for yourselves and store a sizable amount of data inside each file. Also, consider binary save/load for better speed, accuracy, and consistency, though, it will be less readable for humans and portability might suffer a bit - some odd platforms use less common endianess.

  5. You can speed up the loop

    for (size_t i = 0; i < TSIZE; i++) { for (size_t j = 0; j <= i; j++) { pm(j, i) = pm(i, j) = (1.0 / TSIZE) * (m.col(i).dot(m.col(j).transpose())); } } 

    Note: by dot function I expect to see scalar pruduct, so it is odd for me to see that you transpose the vector inside it. Use operator * for matrix multiplication.

  6. I am not too familiar with these Matrices classes, but most likely, it will be better if you replace

    podx.col(i) = podx.col(i) + (1.0 / (eigval(i) * TSIZE)) * sqrt(eigval(i) * TSIZE) * eigvec(j, i) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    with

    podx.col(i) += (eigvec(j, i) / sqrt(eigval(i) * TSIZE)) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    also, what to do if eigval(i) == 0. or too close to it?

  7. If you plan to grow your project you'd better eventually replace std::cout with a usage of a logger class as std::cout is not thread friendly despite being technically thread safe. Also, in this, case move the code inside a class and each relevant section move inside a function with a sensible name.

  1. Instead of #define use enum for the constant numbers.

  2. I don't think that the number of time data or number of points should be absolute constants - rather they should be determined during runtime. Else you have to recompile your code each time you receive new data.

  3. Better declare some sort of output path instead of relying on ../, also you need to make sure that the directories exist, else it will fail to save.

  4. You don't want to store small pieces of data over huge amount of files and directories. Better make a serialization standard for yourselves and store a sizable amount of data inside each file. Also, consider binary save/load for better speed, accuracy, and consistency, though, it will be less readable for humans and portability might suffer a bit - some odd platforms use less endianess.

  5. You can speed up the loop

    for (size_t i = 0; i < TSIZE; i++) { for (size_t j = 0; j <= i; j++) { pm(j, i) = pm(i, j) = (1.0 / TSIZE) * (m.col(i).dot(m.col(j).transpose())); } } 
  6. I am not too familiar with these Matrices classes, but most likely, it will be better if you replace

    podx.col(i) = podx.col(i) + (1.0 / (eigval(i) * TSIZE)) * sqrt(eigval(i) * TSIZE) * eigvec(j, i) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    with

    podx.col(i) += (eigvec(j, i) / sqrt(eigval(i) * TSIZE)) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    also, what to do if eigval(i) == 0. or too close to it?

  7. If you plan to grow your project you'd better eventually replace std::cout with a usage of a logger class as std::cout is not thread friendly despite being technically thread safe. Also, in this, case move the code inside a class and each relevant section move inside a function with a sensible name.

  1. Instead of #define use enum or constexpr for the constant numbers.

  2. I don't think that the number of time data or number of points should be absolute constants - rather they should be determined during runtime. Else you have to recompile your code each time you receive new data.

  3. Better declare some sort of output path instead of relying on ../, also you need to make sure that the directories exist, else it will fail to save.

  4. You don't want to store small pieces of data over huge amount of files and directories. Better make a serialization standard for yourselves and store a sizable amount of data inside each file. Also, consider binary save/load for better speed, accuracy, and consistency, though, it will be less readable for humans and portability might suffer a bit - some odd platforms use less common endianess.

  5. You can speed up the loop

    for (size_t i = 0; i < TSIZE; i++) { for (size_t j = 0; j <= i; j++) { pm(j, i) = pm(i, j) = (1.0 / TSIZE) * (m.col(i).dot(m.col(j).transpose())); } } 

    Note: by dot function I expect to see scalar pruduct, so it is odd for me to see that you transpose the vector inside it. Use operator * for matrix multiplication.

  6. I am not too familiar with these Matrices classes, but most likely, it will be better if you replace

    podx.col(i) = podx.col(i) + (1.0 / (eigval(i) * TSIZE)) * sqrt(eigval(i) * TSIZE) * eigvec(j, i) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    with

    podx.col(i) += (eigvec(j, i) / sqrt(eigval(i) * TSIZE)) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    also, what to do if eigval(i) == 0. or too close to it?

  7. If you plan to grow your project you'd better eventually replace std::cout with a usage of a logger class as std::cout is not thread friendly despite being technically thread safe. Also, in this, case move the code inside a class and each relevant section move inside a function with a sensible name.

Source Link
ALX23z
  • 2.5k
  • 7
  • 15

  1. Instead of #define use enum for the constant numbers.

  2. I don't think that the number of time data or number of points should be absolute constants - rather they should be determined during runtime. Else you have to recompile your code each time you receive new data.

  3. Better declare some sort of output path instead of relying on ../, also you need to make sure that the directories exist, else it will fail to save.

  4. You don't want to store small pieces of data over huge amount of files and directories. Better make a serialization standard for yourselves and store a sizable amount of data inside each file. Also, consider binary save/load for better speed, accuracy, and consistency, though, it will be less readable for humans and portability might suffer a bit - some odd platforms use less endianess.

  5. You can speed up the loop

    for (size_t i = 0; i < TSIZE; i++) { for (size_t j = 0; j <= i; j++) { pm(j, i) = pm(i, j) = (1.0 / TSIZE) * (m.col(i).dot(m.col(j).transpose())); } } 
  6. I am not too familiar with these Matrices classes, but most likely, it will be better if you replace

    podx.col(i) = podx.col(i) + (1.0 / (eigval(i) * TSIZE)) * sqrt(eigval(i) * TSIZE) * eigvec(j, i) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    with

    podx.col(i) += (eigvec(j, i) / sqrt(eigval(i) * TSIZE)) * m.block<MSIZE, 1>(0 * MSIZE, j); 

    also, what to do if eigval(i) == 0. or too close to it?

  7. If you plan to grow your project you'd better eventually replace std::cout with a usage of a logger class as std::cout is not thread friendly despite being technically thread safe. Also, in this, case move the code inside a class and each relevant section move inside a function with a sensible name.