-
Notifications
You must be signed in to change notification settings - Fork 16
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 81.20% 80.84% -0.37%
==========================================
Files 49 47 -2
Lines 2001 1879 -122
Branches 267 257 -10
==========================================
- Hits 1625 1519 -106
+ Misses 376 360 -16
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
src/polysolve/linear/Solver.hpp
Outdated
@@ -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?
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.
{ | ||
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
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.
is_nullspace is not necessary to exist anymore, so it is removed.
{ | ||
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
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 removed.
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
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.
Moved.
#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
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.
#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.