Skip to content

Eigen solution with Number class to compile#309

Open
ohhmm wants to merge 1 commit intomainfrom
eigen
Open

Eigen solution with Number class to compile#309
ohhmm wants to merge 1 commit intomainfrom
eigen

Conversation

@ohhmm
Copy link
Owner

@ohhmm ohhmm commented Sep 21, 2024

No description provided.

@ohhmm ohhmm force-pushed the eigen branch 2 times, most recently from 70d483e to 48c72b5 Compare December 2, 2024 14:12
@ohhmm ohhmm force-pushed the eigen branch 5 times, most recently from 1eec888 to 7e2885e Compare February 18, 2025 22:18
@ohhmm ohhmm force-pushed the eigen branch 3 times, most recently from 888401a to 112fd20 Compare March 1, 2025 20:27
@ohhmm ohhmm force-pushed the eigen branch 2 times, most recently from cf57908 to 5c74a2e Compare March 14, 2025 18:23
// return det;
//}

using extrapolator_base_matrix = Eigen::Matrix<Number, Eigen::Dynamic, Eigen::Dynamic>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple issues found in the Eigen integration:

  1. The Number type is undefined but used in using extrapolator_base_matrix = Eigen::Matrix<Number, Eigen::Dynamic, Eigen::Dynamic>;
  2. The header file "Expression.h" is included but doesn't exist in the codebase
  3. The CMake configuration for Eigen is causing build failures - the repository URL https://github.com/libeigen/eigen.git is incorrect or inaccessible

Recommendations:

  • Define the Number type or use an existing type like Valuable or a numeric type
  • Remove the non-existent "Expression.h" include
  • Fix the Eigen repository URL in CMake configuration or use vcpkg properly

// return det;
//}

using extrapolator_base_matrix = Eigen::Matrix<Number, Eigen::Dynamic, Eigen::Dynamic>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Extrapolator class has conflicting base class definitions:

// First definition
using extrapolator_base_matrix = boost::numeric::ublas::matrix<extrapolator_value_type>;

// Second definition that overwrites the first
using extrapolator_base_matrix = Eigen::Matrix<Number, Eigen::Dynamic, Eigen::Dynamic>;

This creates confusion and compilation errors. You should:

  1. Choose one implementation (either boost::ublas or Eigen)
  2. If transitioning to Eigen, update all related code consistently
  3. Remove the commented-out code that's no longer needed

// }
auto solution = this->llt().solve(a);
return solution.rhs();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of Solve method has several issues:

  1. There's a duplicate implementation of Solve - one is declared but not defined, and another is defined inline
  2. The inline implementation uses Eigen's llt() method but returns solution.rhs() which doesn't match the expected return type
  3. The method doesn't handle potential errors in matrix solving

Recommendation: Consolidate the two implementations into one correct implementation that properly handles errors and returns the expected type.

@ohhmm ohhmm force-pushed the eigen branch 3 times, most recently from 4fb171a to 80a7294 Compare March 21, 2025 20:56
@ohhmm ohhmm force-pushed the eigen branch 5 times, most recently from 50f36fd to b2b3cf1 Compare March 30, 2025 18:14
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

Successfully merging this pull request may close these issues.

1 participant