-
Notifications
You must be signed in to change notification settings - Fork 51
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
Refactor linear algebra functionality for complex-valued problems #47
Conversation
90add8f
to
d29335f
Compare
97a68f8
to
ff10b89
Compare
ab1b35d
to
9f5e774
Compare
dfb66ca
to
ea571b5
Compare
ff10b89
to
64fa330
Compare
ea571b5
to
fd34f79
Compare
64fa330
to
586a147
Compare
bb79f42
to
9bbfe39
Compare
46d0c53
to
77a2be9
Compare
9bbfe39
to
4343d4d
Compare
cba2779
to
af4a5e5
Compare
10c87a4
to
6317e8e
Compare
af4a5e5
to
5573f61
Compare
6317e8e
to
25d276c
Compare
25d276c
to
cc43db8
Compare
…functionality and wave ports
Reverts prior commit as this idea did not work, one boundary attribute may touch many domain attributes.
… S-parameter calculations
…unicator is different than the solvers (for example, many processors with empty local matrices)
9a51e9f
to
393b428
Compare
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 have definitely missed some things as this was a pretty big read, but hopefully I've caught a good chunk.
- For derived virtual classes, consider use of
final
for the class or foroverrides
if there is no intention of a child in future. Same for choosing betweenprotected
andprivate
for member variables and methods. - I have a branch
hughcars/driven-port-debug-comments
where I tried out a few of my suggested changes to check they worked, feel free to take as appropriate. Particularly theComplexVector
value semantics I thiiink are complete on there.
ComplexVector(int n = 0); | ||
|
||
// Copy constructor. | ||
ComplexVector(const ComplexVector &y); |
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.
tl;dr: this class needs to match the general c++ value semantics, so I proposed a few changes. For details, see the cpp and hughcars/driven-port-debug-comments
for an implementation.
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.
As discussed offline, I've added some subset of the functionality suggested in hughcars/driven-port-debug-comments
. For now I've skipped the move constructor since we haven't had a need for it.
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 have definitely missed some things as this was a pretty big read, but hopefully I've caught a good chunk.
- For derived virtual classes, consider use of
final
for the class or foroverrides
if there is no intention of a child in future. Same for choosing betweenprotected
andprivate
for member variables and methods. - I have a branch
hughcars/driven-port-debug-comments
where I tried out a few of my suggested changes to check they worked, feel free to take as appropriate. Particularly theComplexVector
value semantics I thiiink are complete on there.
…nSolver -> Arpack/SlepcEigenvalueSolver, using instead of typedef, fix some typos in comments
…with a mutable RAP member to avoid some const_cast, clarify some comments, use initializer lists where possible for constructors
Initial testing shows the expected 2x improvement in number of iterations, where each iteration now doubles in cost at the fine levels due to complex-valued MVP instead of real-valued. However, for expensive coarse solves, this is a substantial improvement and speedups look to be 25% on cavity example. Also adds back Chebyshev smoothing based on the standard 1st-kind polynomials, with runtime options for configuring which variant and the associated tolerances."
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.
Still disagree on Arpack and the buffers, but not a hill to die on.
Initial testing shows the expected 2x improvement in number of iterations, where each iteration now doubles in cost at the fine levels due to complex-valued MVP instead of real-valued. However, for expensive coarse solves, this is a substantial improvement and speedups look to be 25% on cavity example. Also adds back Chebyshev smoothing based on the standard 1st-kind polynomials, with runtime options for configuring which variant and the associated tolerances."
Initial testing shows the expected 2x improvement in number of iterations, where each iteration now doubles in cost at the fine levels due to complex-valued MVP instead of real-valued. However, for expensive coarse solves, this is a substantial improvement and speedups look to be 25% on cavity example. Also adds back Chebyshev smoothing based on the standard 1st-kind polynomials, with runtime options for configuring which variant and the associated tolerances."
Previously, Palace leveraged PETSc configured with complex-valued scalars to provide support for solving complex-valued linear systems, using wrapper classes like
PetscParMatrix
andPetscParVector
. With this PR, complex-valued linear algebra functionality is built directly on the underlying MFEM data structures, removing the need for PETSc as a direct dependency (only for the SLEPc eigenvalue solver). For small (sequential) complex-valued linear algebra like for PROM construction, we use Eigen.This will make features like operator partial-assembly much easier to implement, as well as simplifying eventual GPU support by avoiding many interfaces between PETSc and MFEM objects.
This PR is a prerequisite for #64. Since #64 contains some follow-up changes to changes also made in this PR, I propose we review both simultaneously and only merge #47 quickly followed by #64 once comments with both have been addressed and they are both approved.
TODO
Closes #42