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

New PR: Eigen system supports shell matrices #15315

Merged
merged 18 commits into from May 20, 2020

Conversation

fdkong
Copy link
Contributor

@fdkong fdkong commented May 19, 2020

Refs #14406

A new PR is required to use the new libmesh and the new PETSc in the civet. I simply move the old PR #14406 here.

  1. Shell matrices for nonlinear and linear solvers.

  2. Allow users to attach a customized preconditioner (with a matrix-free option for preconditioner). Closes Attach preconditioner for Eigen solver #14297

  3. Support array kernels

  4. Add an option (precond_matrix_includes_eigen) to allow users to add eigen kernels into the preconditioning matrix

  5. getConvergedEigenvalue returns only eigenvalues without touching the eigenvectors. Closes Getting eigenvalue in NonlinearEigenSystem changes solution #14506

Closes #14297

@fdkong fdkong changed the base branch from devel to next May 19, 2020 14:59
@moosebuild
Copy link
Contributor

moosebuild commented May 19, 2020

Job Documentation on 4ae1b39 wanted to post the following:

View the site here

This comment will be updated on new commits.

@@ -50,7 +50,8 @@

[Executioner]
type = Eigenvalue
solve_type = MF_MONOLITH_NEWTON
matrix_free = true
Copy link
Member

Choose a reason for hiding this comment

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

This is so much better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

looks like some very neat capability!

@@ -114,7 +122,17 @@ class NonlinearEigenSystem : public NonlinearSystemBase
* is the real and the imaginary part of
* the eigenvalue, respectively.
*/
virtual const std::pair<Real, Real> getNthConvergedEigenvalue(dof_id_type n);
virtual const std::pair<Real, Real> getConvergedEigenvalue(dof_id_type n);
Copy link
Member

Choose a reason for hiding this comment

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

If you're returning by value, is there a point to making the return value const? Also if returning by value, can you make this method const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

framework/include/systems/NonlinearEigenSystem.h Outdated Show resolved Hide resolved
framework/include/systems/NonlinearEigenSystem.h Outdated Show resolved Hide resolved
framework/include/systems/NonlinearEigenSystem.h Outdated Show resolved Hide resolved
framework/include/systems/NonlinearEigenSystem.h Outdated Show resolved Hide resolved
framework/src/systems/NonlinearEigenSystem.C Outdated Show resolved Hide resolved
framework/src/systems/NonlinearEigenSystem.C Outdated Show resolved Hide resolved
framework/src/utils/SlepcSupport.C Outdated Show resolved Hide resolved
framework/src/utils/SlepcSupport.C Outdated Show resolved Hide resolved
framework/src/utils/SlepcSupport.C Outdated Show resolved Hide resolved
@fdkong
Copy link
Contributor Author

fdkong commented May 20, 2020

@lindsayad Hopefully, addressed your comments. Haha

@moosebuild
Copy link
Contributor

Job App tests on 4ae1b39 : invalidated by @fdkong

/bin/sh: 1: /opt/civet/build_0/moose/libmesh/installed/contrib/bin/libmesh-config: not found

@moosebuild
Copy link
Contributor

Job Modules debug on 4ae1b39 : invalidated by @fdkong

Cancelling job due to step taking longer than the max 21600 seconds

@fdkong
Copy link
Contributor Author

fdkong commented May 20, 2020

@lindsayad It is ready to review or merge

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

beautiful code

@lindsayad lindsayad merged commit 8e8e1dc into idaholab:next May 20, 2020
fdkong added a commit to fdkong/moose that referenced this pull request May 20, 2020
/opt/civet/build_0/moose/framework/src/utils/SlepcSupport.C:385:40: error: this statement may fall through [-Werror=implicit-fallthrough=]
  385 |       solver_params._eigen_matrix_free = true;
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
/opt/civet/build_0/moose/framework/src/utils/SlepcSupport.C:387:5: note: here
  387 |     case Moose::EST_NEWTON:
      |     ^~~~
cc1plus: all warnings being treated as errors

We changed APIs in  idaholab#15315, and we need to reserve the old APIs for Griffin for a second.

I will go to update Griffin to use new APIs. Once that is done, we could remove the old APIs.

Refs idaholab#15315
tophmatthews pushed a commit to tophmatthews/moose that referenced this pull request May 27, 2020
/opt/civet/build_0/moose/framework/src/utils/SlepcSupport.C:385:40: error: this statement may fall through [-Werror=implicit-fallthrough=]
  385 |       solver_params._eigen_matrix_free = true;
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
/opt/civet/build_0/moose/framework/src/utils/SlepcSupport.C:387:5: note: here
  387 |     case Moose::EST_NEWTON:
      |     ^~~~
cc1plus: all warnings being treated as errors

We changed APIs in  idaholab#15315, and we need to reserve the old APIs for Griffin for a second.

I will go to update Griffin to use new APIs. Once that is done, we could remove the old APIs.

Refs idaholab#15315
fdkong added a commit to fdkong/moose that referenced this pull request Jun 8, 2020
fdkong added a commit to fdkong/moose that referenced this pull request Jun 8, 2020
@YaqiWang
Copy link
Contributor

YaqiWang commented Aug 2, 2020

@pbalest and @amherm reported an issue to me that an input in Griffin worked before but stopped working. It is not so obvious how that can stop working so I had to spend quite some time on bisection and finally located the trouble some in Griffin, which is a MOOSE submodule update from 22ad65b to 983f0ab. There are bunch of things going on during these two hashes, but this is one is the most susceptible.

@fdkong
Copy link
Contributor Author

fdkong commented Aug 2, 2020

@pbalest and @amherm reported an issue to me that an input in Griffin worked before but stopped working. It is not so obvious how that can stop working so I had to spend quite some time on bisection and finally located the trouble some in Griffin, which is a MOOSE submodule update from 22ad65b to 983f0ab. There are bunch of things going on during these two hashes, but this is one is the most susceptible.

I checked PR a while ago and did not see anything wrong. Do you have a moose-only input file?

@YaqiWang
Copy link
Contributor

YaqiWang commented Aug 2, 2020

No. It is a complicated Griffin input. An eigenvalue problem with flux coupled with delayed neutron precursor drift. The capability is tested in Griffin but the test works fine. I have no idea how this can break that input. I can send you the input to you. The hash in Griffin from working to non-working is 47473250 -> 26589755.

@YaqiWang
Copy link
Contributor

YaqiWang commented Aug 2, 2020

I am checking out moose hash 8e8e1dc, the merge commit of this PR to make sure it is indeed this one causing trouble.

@fdkong
Copy link
Contributor Author

fdkong commented Aug 2, 2020

Could you add a griffin branch or PR? So I could just grab your branch.

@YaqiWang
Copy link
Contributor

YaqiWang commented Aug 2, 2020

Ok, let me add that input into a branch.

@YaqiWang
Copy link
Contributor

YaqiWang commented Aug 2, 2020

Please grab my msr_problemtic_input branch. The top commit is the input (examples/msr/diff.i). It is based on the first not-working hash 265889755 in Griffin.

@YaqiWang
Copy link
Contributor

YaqiWang commented Aug 2, 2020

Wait, this hash (8e8e1dc) seems working ok. Could be something else breaking the input. Or maybe I need to do a make clobberall...

@@ -137,7 +137,7 @@ PhysicsBasedPreconditioner::PhysicsBasedPreconditioner(const InputParameters & p
for (unsigned int var = 0; var < n_vars; var++)
addSystem(var, off_diag[var], _pre_type[var]);

_nl.nonlinearSolver()->attach_preconditioner(this);
_nl.attachPreconditioner(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

@fdkong there are several places in Griffin that need to be patched like this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will take a look

@GiudGiud
Copy link
Contributor

@amherm could you please get in touch with me or Alex.
We'd like to have your name for the acknowledgements in a paper and it's not listed on Github
thank you!

@amherm
Copy link
Contributor

amherm commented Jun 14, 2022 via email

@GiudGiud
Copy link
Contributor

Ok got your name, that's all I wanted. I should have guessed. Thanks Andrew!

lindsayad added a commit to lindsayad/moose that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting eigenvalue in NonlinearEigenSystem changes solution Attach preconditioner for Eigen solver
7 participants