-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add Trilinos for linear solver #69
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you try to add to the ci (UNIX only) trilinos installed with apt?
@@ -153,6 +155,16 @@ | |||
], | |||
"doc": "Settings for the AMGCL solver." | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think you need any of these, there are no options for trilinos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to add the options from trilinos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/polysolve/linear/Solver.cpp
Outdated
Eigen::MatrixXd test_vertices; | ||
Eigen::MatrixXd init_vertices; | ||
std::vector<int> test_boundary_nodes; | ||
Eigen::MatrixXd remove_boundary_vertices(const Eigen::MatrixXd &vertices, const std::vector<int> &boundary_nodes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
src/polysolve/linear/Solver.hpp
Outdated
@@ -25,6 +34,11 @@ namespace spdlog | |||
|
|||
namespace polysolve::linear | |||
{ | |||
#ifdef POLYSOLVE_LARGE_INDEX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
{ | ||
if (params.contains("Trilinos")) | ||
{ | ||
if (params["Trilinos"].contains("block_size")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add these options to the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now the block size is a method
src/polysolve/linear/Solver.hpp
Outdated
extern Eigen::MatrixXd init_vertices; | ||
extern Eigen::MatrixXd test_vertices; | ||
extern std::vector<int> test_boundary_nodes; | ||
Eigen::MatrixXd remove_boundary_vertices(const Eigen::MatrixXd &vertices, const std::vector<int> &boundary_nodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be inside trilinos, not here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
+ Coverage 87.27% 87.63% +0.35%
==========================================
Files 48 46 -2
Lines 2028 1916 -112
==========================================
- Hits 1770 1679 -91
+ Misses 258 237 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need unit test, with and without block size and null space
@@ -77,9 +77,11 @@ option(POLYSOLVE_WITH_SUPERLU "Enable SuperLU library" | |||
option(POLYSOLVE_WITH_MKL "Enable MKL library" ${POLYSOLVE_NOT_ON_APPLE_SILICON}) | |||
option(POLYSOLVE_WITH_CUSOLVER "Enable cuSOLVER library" OFF) | |||
option(POLYSOLVE_WITH_PARDISO "Enable Pardiso library" OFF) | |||
option(POLYSOLVE_WITH_HYPRE "Enable hypre" ON) | |||
option(POLYSOLVE_WITH_HYPRE "Enable hypre" OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on
@@ -4,6 +4,10 @@ | |||
|
|||
#include <memory> | |||
|
|||
#include <Eigen/Dense> | |||
#include <Eigen/Sparse> | |||
#include <vector> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are not necessary?
{ | ||
if (params.contains("Trilinos")) | ||
{ | ||
if (params["Trilinos"].contains("block_size")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now the block size is a method
{ | ||
conv_tol_ = params["Trilinos"]["tolerance"]; | ||
} | ||
if (params["Trilinos"].contains("is_nullspace")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also a method
{ | ||
if (test_vertices.cols()==3) | ||
{ | ||
reduced_vertices=remove_boundary_vertices(test_vertices,test_boundary_nodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shoudnt be here, the null space is a method
Eigen::MatrixXd test_vertices; | ||
Eigen::MatrixXd init_vertices; | ||
std::vector<int> test_boundary_nodes; | ||
Eigen::MatrixXd remove_boundary_vertices(const Eigen::MatrixXd &vertices, const std::vector<int> &boundary_nodes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be inside an util file
#include <Eigen/Sparse> | ||
#include <vector> | ||
|
||
#ifdef HAVE_MPI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mpi is always here?
// column-major matrix, the solver will actually solve A^T x = b. | ||
// | ||
|
||
extern Eigen::MatrixXd init_vertices; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no extern, this is not part of the api
#include "Teuchos_CommandLineProcessor.hpp" | ||
#include "Epetra_Map.h" | ||
#include "Epetra_Vector.h" | ||
#include "Epetra_CrsMatrix.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need all these includes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are all pointers, we could forward declare
No description provided.