idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.7k stars 1.04k forks source link

Steady and eigen executioners do not have Picard iteration #9038

Closed YaqiWang closed 5 years ago

YaqiWang commented 7 years ago

Description of the enhancement or error report

Should we add a simple outer Picard loop?

Rationale for the enhancement or information for reproducing the error

This is required by some of the steady-state multiphysics simulations. Also if this is in place, codes in Rattlesnake for doing nonlinear diffusion acceleration can be cleaned up.

Identified impact

(i.e. Internal object changes, limited interface changes, public API change, or a list of specific applications impacted) New capability so no impact on the current capabilities.

permcody commented 7 years ago

Hmm, I suppose this is fine for now. The grand plan of having a single Executioner is not in the works at the moment so if you need an outer Picard loop in either or both of those Executioners, we should just add it.

vincentlaboure commented 5 years ago

We are planning to address this issue in the near future in order to remove some of the executioners we have in Rattlesnake.

@permcody @friedmud Before we start putting time into this, do you foresee any reservations you may have if we added Picard iterations to the Steady executioner?

permcody commented 5 years ago

I don't really care, however, we've long talked about just getting rid of the Steady Executioner to reduce code duplication and maintenance. The Steady executioner doesn't really do anything special beyond what the Transient does besides check for time kernels. I'm changing several things with Picard iterations right now in the Transient. I'd really hate to have to do that with Steady after fixing Transient.

YaqiWang commented 5 years ago

Can we move Picard iteration into FEProblem::solve? It is overall part of the solve anyway.

permcody commented 5 years ago

Maybe - I honestly think the easier solution is to get rid of Steady. We can probably alias it and set special parameters so that it'll look the same.

On Wed, Jan 16, 2019 at 9:13 AM Yaqi notifications@github.com wrote:

Can we move Picard iteration into FEProblem::solve? It is overall part of the solve anyway.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/9038#issuecomment-454838418, or mute the thread https://github.com/notifications/unsubscribe-auth/AC5XILy3syLJByI7Znot5ilRYWMJBJIgks5vD0_EgaJpZM4NNeWf .

friedmud commented 5 years ago

Steady needs to go.

As for moving the Picard stuff... it's an interesting idea - but I'm not sure it will work.

On Wed, Jan 16, 2019 at 9:18 AM Cody Permann notifications@github.com wrote:

Maybe - I honestly think the easier solution is to get rid of Steady. We can probably alias it and set special parameters so that it'll look the same.

On Wed, Jan 16, 2019 at 9:13 AM Yaqi notifications@github.com wrote:

Can we move Picard iteration into FEProblem::solve? It is overall part of the solve anyway.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/9038#issuecomment-454838418, or mute the thread < https://github.com/notifications/unsubscribe-auth/AC5XILy3syLJByI7Znot5ilRYWMJBJIgks5vD0_EgaJpZM4NNeWf

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/9038#issuecomment-454840237, or mute the thread https://github.com/notifications/unsubscribe-auth/AA1JMapocLUXvBMBXSjXKxzxXmMdsN1Eks5vD1DHgaJpZM4NNeWf .

YaqiWang commented 5 years ago

I am not objecting one single executioner although I do think separate Steady, Eigenvalue out from Transient can make things clearer, make the code less confusing and easier to maintain. We need this because we need to do Picard iteration on eigenvalue calculations with the new Eigenvalue executioner, which is required for updating Rattlesnake to the new eigen executioner. Several recent multiphysics calculation tasks demand this update and we have to finish it pretty quickly. Time is our concern.

YaqiWang commented 5 years ago

Looking into the code, it looks to me that moving the Picard iteration inside FEProblem can clean up the code significantly. It can detangle time steppers from the solve and make time stepper refactoring much easier. It requires lots of code shuffling and touches the center of the execution but it ought to be doable. I think we should pursue it asap. It deserves a separate issue.

permcody commented 5 years ago

I don't doubt that it can be done code-wise. The issues that would arise would be related to object design and encapsulation. While It's true that probably 90% of what's in the Executioner is the direct manipulation of the FEProblem class, there are probably just a few things that are related to the "App" or other objects that don't quite make sense when moved into the problem. I do agree that it's worth looking at but it's not going to be a trivial change. It will almost certainly break any derived Executioners that we have in applications currently so it's going to be difficult.

We had talked about redesigning the Executioner class from the ground up about 3 years ago and Derek even made an attempt (his commits are still laying around on Github somewhere) but it's going to be really difficult to actually get it in. The MOOSE team is really constrained as several of us are directly working on application support this year more than ever. It's getting harder to do longer refactoring tasks.

Let me think about what it would take to drop Steady, I really think that's going to be easier.