Skip to content

Refactor HandoverStorage to use Eigen::RowMatrixXd for improved performance#6

Open
jungdaesuh wants to merge 7 commits intomainfrom
handover-storage-eigen-clean
Open

Refactor HandoverStorage to use Eigen::RowMatrixXd for improved performance#6
jungdaesuh wants to merge 7 commits intomainfrom
handover-storage-eigen-clean

Conversation

@jungdaesuh
Copy link
Owner

Summary

  • Replace nested std::vector<std::vector<double>> with Eigen::RowMatrixXd for improved cache locality and reduced pointer chasing in the radial preconditioner / tri-diagonal solver
  • Only allocate lthreed-dependent arrays when lthreed=true, saving memory for axisymmetric (2D) cases
  • Use static_cast<std::size_t> in accessor pointer arithmetic to prevent potential integer overflow for large mn * ns products

This PR addresses all feedback from proximafusion#394:

  • Uses Eigen::RowMatrixXd instead of flat vectors with custom indexers
  • No benchmark files or test artifacts
  • Clean commit history from upstream main

Test plan

  • All C++ Bazel tests pass
  • All Python tests pass
  • Code builds cleanly with CMake

…rmance

- Migrated HandoverStorage to use Eigen::RowMatrixXd
- Cleaned up unnecessary comments from Eigen migration
@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


// call serial Thomas solver for every mode number individually
// Uses span accessors to pass contiguous radial slices to the solver
const int ns = m_h_.all_ar.cols();
Copy link

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'Index' (aka 'long') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]

  const int ns = m_h_.all_ar.cols();
                 ^

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.

3 participants