Skip to content
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

Tupek/trust region subspace solver #1288

Merged
merged 25 commits into from
Dec 20, 2024
Merged

Conversation

tupek2
Copy link
Collaborator

@tupek2 tupek2 commented Dec 10, 2024

No description provided.

@tupek2
Copy link
Collaborator Author

tupek2 commented Dec 10, 2024

/style

@tupek2 tupek2 force-pushed the tupek/trust_region_subspace_solver branch from c8d6425 to 92e28a2 Compare December 11, 2024 04:59
@tupek2
Copy link
Collaborator Author

tupek2 commented Dec 11, 2024

/style

1 similar comment
@tupek2
Copy link
Collaborator Author

tupek2 commented Dec 11, 2024

/style

@tupek2 tupek2 requested a review from btalamini December 12, 2024 15:06
src/serac/numerics/dense_petsc.hpp Outdated Show resolved Hide resolved
src/serac/numerics/dense_petsc.hpp Outdated Show resolved Hide resolved
src/serac/numerics/dense_petsc.hpp Outdated Show resolved Hide resolved
src/serac/numerics/dense_petsc.hpp Show resolved Hide resolved
src/serac/numerics/dense_petsc.hpp Outdated Show resolved Hide resolved
src/serac/numerics/equation_solver.cpp Outdated Show resolved Hide resolved
src/serac/numerics/equation_solver.cpp Outdated Show resolved Hide resolved
src/serac/numerics/equation_solver.cpp Show resolved Hide resolved
src/serac/numerics/tests/test_trust_region_solver.cpp Outdated Show resolved Hide resolved
src/serac/numerics/equation_solver.cpp Show resolved Hide resolved
@tupek2
Copy link
Collaborator Author

tupek2 commented Dec 16, 2024

/style

@tupek2 tupek2 requested review from white238 and chapman39 December 16, 2024 18:35
@chapman39
Copy link
Collaborator

chapman39 commented Dec 16, 2024

im not too familiar with petsc/ mfem, but could a lot of the petsc wrapper stuff use mfem's petsc wrapper classes? https://github.com/mfem/mfem/blob/master/linalg/petsc.hpp ?

also https://github.com/mfem/mfem/blob/master/linalg/slepc.hpp

@tupek2
Copy link
Collaborator Author

tupek2 commented Dec 17, 2024

The mfem wrappers are intended for 'parallel' across processor things. The new wrapper is for local, small, dense things. In general, many coding experts recommend creating internal abstractions to external library wrappers anyways, but in this case, the mfem one doesn't even really fit the purpose.

@tupek2 tupek2 force-pushed the tupek/trust_region_subspace_solver branch from 0858469 to f7164f3 Compare December 17, 2024 00:24
@tupek2
Copy link
Collaborator Author

tupek2 commented Dec 17, 2024

/style

@tupek2 tupek2 force-pushed the tupek/trust_region_subspace_solver branch from 0da6a06 to b0e61ee Compare December 18, 2024 21:12
Copy link
Collaborator

@chapman39 chapman39 left a comment

Choose a reason for hiding this comment

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

thanks @tupek2

auto eigh(const DenseMat& Adense)
{
auto [isize, jsize] = Adense.size();
SLIC_ERROR_IF(isize != jsize, "Eig must be called for symmetric matrices");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SLIC_ERROR_IF(isize != jsize, "Eig must be called for symmetric matrices");
SLIC_ERROR_IF(isize != jsize, "'eigh' must be called for symmetric matrices");

?

* @brief compute the quadratic energy from small dense matrices and vectors
* @param A The stiffness matrix
* @param b The rhs vector
* @param x The current solution vector
Copy link
Member

Choose a reason for hiding this comment

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

Does the doxygen check not throw a warning/error on missing the return parameter documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it does not seem to error. Presumably our functions should be named such that the return is often obvious.

@tupek2
Copy link
Collaborator Author

tupek2 commented Dec 20, 2024

/style

@tupek2 tupek2 merged commit bd75d9c into develop Dec 20, 2024
2 checks passed
@tupek2 tupek2 deleted the tupek/trust_region_subspace_solver branch January 8, 2025 00:01
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.

4 participants