-
Notifications
You must be signed in to change notification settings - Fork 247
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
[LinearSolvers] new solver for (symmetric) generalized eigenproblems using the spectra library #8440
Conversation
@@ -16,7 +16,7 @@ message( "**** configuring KratosLinearSolversApplication ****" ) | |||
################### PYBIND11 | |||
include(pybind11Tools) | |||
|
|||
add_definitions( -DEIGEN_DEFAULT_TO_ROW_MAJOR -DEIGEN_MPL2_ONLY ) | |||
# TODO add_definitions( -DEIGEN_MPL2_ONLY) |
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.
Spectra internally uses the Eigen::Cholesky, that however is not available with MPL2_ONLY. I will try to write an interface to another Eigen solver instead, ev, even MKLEdit: Cholesky is not needed, the MPL2 flag can remain unchanged.- We have tried Spectra before without success: I think it was because it does not work with
-DEIGEN_DEFAULT_TO_ROW_MAJOR
. That does not have to be used however, as long as we specify the RowMajor ordering explicitly when interfacing Eigen matrices with Kratos matrices.
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.
dear @armingeiser i see myself obliged to block this PR since we are attempting to be restrictive with new libraries to be included.
Can u describe what is this library and why it is needed?
what is its license?
just to add that adding a library to this Application is even more restricted since it is a default app used by everyone else |
@RiccardoRossi i have updated the description, that should hopefully answer your questions. |
@philbucher @emiroglu feel free to try this solver with your problems, would be interesting to see how it compares to the existing |
Does anyone have an idea why compilation works with gcc but not with clang or on windows? The error is always similar:
|
do you need the guards? |
Yes. Thanks for the hint. should be solved now. I have not activted spectra for centos, windows, clang and nightly - since they also did not have MKL or Feast activated. Could be done however if desired. |
@armingeiser do you remember the problems we had some years ago? Are they solved? |
@@ -80,7 +80,7 @@ def _create_mechanical_solution_strategy(self): | |||
computing_model_part = self.GetComputingModelPart() | |||
|
|||
solver_type = self.settings["eigensolver_settings"]["solver_type"].GetString() | |||
if solver_type == "eigen_eigensystem": | |||
if solver_type in ["eigen_eigensystem", "spectra_sym_g_eigs_shift"]: # TODO evaluate what has to be used for spectra |
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.
ping did you confirm 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.
not yet, good catch.
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 did you call it spectra1?
- spectra offers many more solvers right? Do you have plans to expose those too?
applications/LinearSolversApplication/tests/test_spectra_eigensystem_solver.py
Outdated
Show resolved
Hide resolved
applications/LinearSolversApplication/tests/test_spectra_eigensystem_solver.py
Outdated
Show resolved
Hide resolved
I just checked this with my shell turbine model (~415K dofs) and it is much faster that the current solver @KratosMultiphysics/technical-committee I recommend to approve this, this is a very valuable addition IMO |
decision reached in @technicalcommittee
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.
approving on behalf of @KratosMultiphysics/technical-committee , on the assuption that it has the same license as Eigen
@philbucher I have updated the PR with the things we discussed last time:
|
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.
nice work!
Please use the Kratos error macro to also get the stack trace when throwing, otherwise very nice!
template<typename _Scalar> using EigenDynamicMatrix = Eigen::Matrix<_Scalar, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor>; | ||
template<typename _Scalar> using EigenDynamicVector = Eigen::Matrix<_Scalar, Eigen::Dynamic, 1>; | ||
|
||
template<typename _Scalar> using EigenSparseMatrix = Eigen::SparseMatrix<_Scalar, Eigen::RowMajor, int>; |
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.
not for this PR but why int
and not size_t
?
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.
Pardiso is expecting signed integers. Therefore the max size of a matrix is ‚only’ 2,147,483,647 which should be more then enough. Eigen is also using signed ints for dense matrices as unsigned ints are very error prone for algorithms.
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.
that means that with shell elements we are limited to ~ 350 nodes (6dofs/node) which I agree is pretty big :)
applications/LinearSolversApplication/custom_solvers/spectra_sym_g_eigs_shift_solver.h
Outdated
Show resolved
Hide resolved
applications/LinearSolversApplication/custom_solvers/spectra_sym_g_eigs_shift_solver.h
Outdated
Show resolved
Hide resolved
eigvecs = eigs.eigenvectors().transpose().colwise().reverse(); | ||
|
||
// --- normalization | ||
// TODO seems to be normalized by Spectra already -> confirm! |
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.
it would be nice if you could take a look in a followup PR
|
||
OpType op(a, b); | ||
BOpType Bop(b); | ||
const int ncv = 3 * nroot; // TODO find a good value |
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.
same
this is very cool! |
…ym_g_eigs_shift_solver.h Co-authored-by: Philipp Bucher <[email protected]>
…ym_g_eigs_shift_solver.h Co-authored-by: Philipp Bucher <[email protected]>
Because this is the current master branch, which will soon become spectra version 1.0 (yixuan/spectra#113)
It has many more features, however, currently i don't have any plans to further extend. |
namespace Kratos | ||
{ | ||
|
||
// IMPORTANT: |
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.
It is the other way round no?
Eigen default is column major while Kratos is row major
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 the comment
guys, our matrices are row_major...
Riccardo
…On Thu, Apr 15, 2021, 9:08 AM Philipp Bucher ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In applications/LinearSolversApplication/linear_solvers_define.h
<#8440 (comment)>
:
> +{
+
+// IMPORTANT:
+//
+// The matrices in Kratos use colum-major storage ordering.
+// Eigen matrices, however, use row-major storage ordering by default.
+//
+// If these two worlds are combined, it is crucial that Eigen matrices are used
+// with **column-major** storage order to match the Kratos matrices!
+//
+// To simplify things, the most commonly used types are defined below:
+
+template<typename _Scalar> using EigenDynamicMatrix = Eigen::Matrix<_Scalar, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor>;
+template<typename _Scalar> using EigenDynamicVector = Eigen::Matrix<_Scalar, Eigen::Dynamic, 1>;
+
+template<typename _Scalar> using EigenSparseMatrix = Eigen::SparseMatrix<_Scalar, Eigen::RowMajor, int>;
that means that with shell elements we are limited to ~ 350 nodes
(6dofs/node) which I agree is pretty big :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8440 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB5PWEKFMJ56NMYTIKHZC7LTI2GILANCNFSM4YV55D4A>
.
|
Seems like you didn’t check the latest changes can you check master? |
Description
Inclusion of the spectra lib and a interface to its more efficient solver for generalized eigensystems of real symmetric matrices.
What is Spectra
In a very first test it seems to be faster then the
eigen_eigensystem
(except if only 1-2 eigenvalues are searched)eigen_eigensystem
[s]spectra_sym_g_eigs_shift
[s]The speed up becomes more prominent when more then just a few eigenvalues are searched, since that is a known limitation of the
eigen_eigensystem
solver - see #2286Changelog
Spectra
to LinearSolversApplicationspectra_sym_g_eigs_shift
-DEIGEN_DEFAULT_TO_ROW_MAJOR
and explicitly specify ordering in Eigen types when interfacing with KRatos matrices.