Non-linear iterations within a physical time step for the advection term #1226
Conversation
itopcuoglu
force-pushed
the
nonlin_dev
branch
from
1932905
to
1b0ed8e
Compare
| @@ -190,11 +190,14 @@ struct AdvectionOp<ICNS, fvm::Godunov> | |||
| m_allow_inflow_on_outflow = true; | |||
| } | |||
|
|
|||
| // if state is NPH, then n and n+1 are known, and only | |||
| // spatial extrapolation is performed | |||
| amrex::Real dt_arg = (fstate == FieldState::NPH) ? 0.0 : dt; | |||
There was a problem hiding this comment.
let's give this a better name than dt_arg. dt_for_hydro ? dt_local ? dt_adjusted?
There was a problem hiding this comment.
Maybe dt_extrap as in dt for extrapolation?
There was a problem hiding this comment.
Changed this into dt_extrap, as in the dt that is used for extrapolation.
| @@ -168,6 +168,10 @@ struct AdvectionOp< | |||
| (godunov_scheme == godunov::scheme::WENO_JS)) { | |||
| m_allow_inflow_on_outflow = true; | |||
| } | |||
| // if state is NPH, then n and n+1 are known, and only | |||
| // spatial extrapolation is performed | |||
| amrex::Real dt_arg = (fstate == FieldState::NPH) ? 0.0 : dt; | |||
There was a problem hiding this comment.
same comment below about naming
There was a problem hiding this comment.
Changed this into dt_extrap as well.
| @@ -172,6 +174,9 @@ private: | |||
| // Prescribe advection velocity | |||
| bool m_prescribe_vel = false; | |||
|
|
|||
| // Advection iterations every timestep | |||
There was a problem hiding this comment.
maybe something with non_linear_iterations in the name? I am a bit wary of our naming for these things because we often mean different things when we say "nonlinear". I am not sure what the right name is but we need to be clear and make sure we stick with it. I have no problem with long, explicit names: e.g. m_non_linear_advection_iterations. Let's add some documentation too.
There was a problem hiding this comment.
We could go for something like m_outer_loop_iterations. I am going to modify the loops to make this compatible with exawind-driver, and I think that is going to make it clear that these are outer loop iterations
| @@ -372,6 +378,10 @@ void incflo::init_physics_and_pde() | |||
| if (activate_overset) { | |||
| m_sim.activate_overset(); | |||
| } | |||
|
|
|||
| // Get number of advection iterations | |||
| pp.query("advection_iterations", m_adv_iters); | |||
There was a problem hiding this comment.
and when we update the naming, make sure the user facing input makes sense within that context as well.
amr-wind/incflo.cpp
Outdated
|
|
||
| // Get number of advection iterations | ||
| pp.query("advection_iterations", m_adv_iters); | ||
| m_adv_iters = std::max(1, m_adv_iters); |
There was a problem hiding this comment.
do we want to silently do this? Or abort on an invalid value? Or warn that we are doing this?
There was a problem hiding this comment.
Got rid of this.
amr-wind/utilities/console_io.cpp
Outdated
| const auto& velstar = vel_star(lev); | ||
| auto& veldiff = vel_diff(lev); | ||
|
|
||
| for (amrex::MFIter mfi(velocity_new(lev)); mfi.isValid(); ++mfi) { |
There was a problem hiding this comment.
Is this possible to do since level_mask is a MultiFab and not a field? Should level_mask be converted into an IntScratchField in order to be accessible by a fused MFIter loop?
There was a problem hiding this comment.
fused mfiter operate on multifabs. no need for fields.
There was a problem hiding this comment.
I converted this into a fused MFIter.
amr-wind/utilities/console_io.cpp
Outdated
| } | ||
| } | ||
|
|
||
| amrex::Real rms_ucell = 0.0; |
There was a problem hiding this comment.
use amrex::Array of size spacedim, stuff the norms into there with a component loop.
There was a problem hiding this comment.
This has been fixed.
amr-wind/utilities/console_io.cpp
Outdated
| rms_vcell += vel_diff(lev).norm2(1); | ||
| rms_wcell += vel_diff(lev).norm2(2); | ||
| } | ||
| amrex::Print() << "Non-linear residual for u velocity " << rms_ucell |
There was a problem hiding this comment.
for loop these, or print as a single line.
There was a problem hiding this comment.
Also make sure the answer is the same when using MPI and multiple procs
There was a problem hiding this comment.
I changed this into printing a single line.
The non-linear solver residuals are the same down to roughly the 5th decimal between 4, 6 and 8 ranks.
| @@ -214,4 +215,75 @@ void print_tpls(std::ostream& out) | |||
| } | |||
| } | |||
|
|
|||
| void print_nonlinear_residual( | |||
There was a problem hiding this comment.
I am not sure this print function belongs here. It feels different than the others. Not sure where to put it yet. I wonder if we can also make it more generic by templating to accept fields. The fields should also have the repo/mesh info so we don't need to pass in sim I think. Maybe this is more of a field utils kinda thing.
|
I don't have anything to add to Marc's remaining comments. This looks really good, I'm impressed with how simply and directly you have managed to set this up. |
…linear iterations using the Godunov scheme
- Calculation of NPH velocity within the non-linear iteration loop
…_nonlinear_residual()
…NPH velocity before the scalar equations
… compatible with exawind-driver
… replaced the indivudal change-in-velocity norms with an Array<amrex::Real, AMREX_SPACEDIM> instance
…rNonLinear(), and moved their functionality into advance() and ApplyPredictor()
itopcuoglu
force-pushed
the
nonlin_dev
branch
from
9881500
to
508bd25
Compare
No description provided.