Conversation
svenn-t
left a comment
There was a problem hiding this comment.
The implementation in the sequential temperature model looks good. Some questions for the black oil model convergence check.
| const bool use_drs_tol = this->param_.tolerance_max_drs_ > 0.0; | ||
| const bool use_drv_tol = this->param_.tolerance_max_drv_ > 0.0; | ||
| const bool use_dsol_tol = use_dp_tol || use_ds_tol || use_drs_tol || use_drv_tol; | ||
| const bool use_dtemp_tol = this->param_.tolerance_max_dtemp_ > 0.0; |
There was a problem hiding this comment.
Some boolean indicating if the temperature model is enable is needed here?
| (!use_drs_tol || (maxSolUpd.dRsMax > 0.0 && maxSolUpd.dRsMax < this->param_.tolerance_max_drs_)) && | ||
| (!use_drv_tol || (maxSolUpd.dRvMax > 0.0 && maxSolUpd.dRvMax < this->param_.tolerance_max_drv_)); | ||
| (!use_drv_tol || (maxSolUpd.dRvMax > 0.0 && maxSolUpd.dRvMax < this->param_.tolerance_max_drv_)) && | ||
| (!use_dtemp_tol || (maxSolUpd.dTempMax > 0.0 && maxSolUpd.dTempMax < this->param_.tolerance_max_dtemp_)); |
There was a problem hiding this comment.
Should this check lead to relaxed CNV for energy as well? I guess there is a difference between fully implicit or sequential temperature model on how this should be handled?
There was a problem hiding this comment.
Aren't we doing that now? use_relaxed_cnv ? is also checked for cnv_energy
There was a problem hiding this comment.
I was a bit unclear. I meant setting tol_cnv_energy to 1e20 when dTempMax check is fulfilled, similar to what we do for tol_cnv with the line Scalar tolerance_cnv_relaxed = relax_dsol_cnv ? 1e20 : param_.tolerance_cnv_relaxed_;
| msg += ") "; | ||
| } | ||
|
|
||
| if (use_dsol_tol) { |
There was a problem hiding this comment.
Add DT output (if fully implicit?)
| print_dsol(use_dp_tol, maxSolUpd.dPMax); | ||
| print_dsol(use_ds_tol, maxSolUpd.dSMax); | ||
| print_dsol(use_drs_tol, maxSolUpd.dRsMax); | ||
| print_dsol(use_drv_tol, maxSolUpd.dRvMax); |
There was a problem hiding this comment.
Add DT output (if fully implicit?)
| if (!isNumericalAquiferCell(elem)) { | ||
| if (scaled_norm > tolerance_cnv_energy_strict) { | ||
| sum_pv_not_converged += pvValue; | ||
| dTMax = max(dTMax, std::abs(dT_[globI])); |
There was a problem hiding this comment.
Don't no if it matters, but could you use std::max?
| struct ToleranceMaxDrv { static constexpr Scalar value = 0.0; }; | ||
|
|
||
| template<class Scalar> | ||
| struct ToleranceMaxDTemp { static constexpr Scalar value = 0.0; }; |
There was a problem hiding this comment.
This leads to command line option name "--tolerance-max-d-temp"
db0e7c9 to
5ce97b8
Compare
|
The updated changes look good to me! |
|
|
||
| const bool use_dsol_tol = use_dp_tol || use_ds_tol || use_drs_tol || use_drv_tol || use_dtemp_tol; | ||
| bool relax_dsol_cnv = false; | ||
| bool relax_energy_cnv = false; |
There was a problem hiding this comment.
As jenkins pointed out, relax_energy_cnv is unused
Depends on OPM/opm-common#4924