Add relaxed mb tolerance parameter #5173
Conversation
|
jenkins build this please |
|
I think this is a good idea, and the goal of stricter default tolerance is good. But I am not sure the "strict for X iterations" approach is good, or user friendly. The behaviour I would seek is to work with the strict tolerance until we have no more iterations (i.e. we are going to say not converged in the current code), then check if the relaxed tolerance is fulfilled, and if it is accept the step and maybe log a warning. I suggest the following: default the min strict mb iterations to -1, and take that to mean the above behaviour, this would let the user override it completely to a different iteration number if necessary. Also, the Jenkins failure is a little strange to me, I would expect 0 differences on all cases when not actively setting this parameter? |
Good idea. Will update. The failed test runs with --tolerance-mb=1.e-7. |
|
jenkins build this please |
I have updated as you suggested. This may effect results though. |
It should not as long as the tolerances are identical? |
Good point. But if you look closely I included the relaxed CNV, which are already different. |
|
benchmark please |
|
Results did not travel back here either:
|
|
Benchmark result overview:
|
| use_relaxed_mb = true; | ||
| use_relaxed_cnv = true; | ||
| if ( terminal_output_ ) { | ||
| std::ostringstream ss; |
There was a problem hiding this comment.
I suggest using fmt::format instead, that would simplify this as you do not have to worry about restoring flags and precision etc.
There was a problem hiding this comment.
I have updated accordingly.
| const bool use_relaxed = cnvErrorPvFraction < param_.relaxed_max_pv_fraction_ && iteration >= param_.min_strict_cnv_iter_; | ||
| const double tol_cnv = use_relaxed ? param_.tolerance_cnv_relaxed_ : param_.tolerance_cnv_; | ||
| bool use_relaxed_cnv = cnvErrorPvFraction < param_.relaxed_max_pv_fraction_ && iteration >= param_.min_strict_cnv_iter_; | ||
| bool use_relaxed_mb = param_.min_strict_mb_iter_ < 0? false : iteration >= param_.min_strict_mb_iter_; |
There was a problem hiding this comment.
Nitpick: space around ?. However, I think the logic here is a little too complicated, and also slightly flawed. With this code, even if I set the number of strict iterations to 1000, I will still get a relaxed final iteration, but that should only happen with -1. Also, it is better to keep these bools const. I suggest:
const bool relax_final_iteration = (param_.min_strict_mb_iter_ < 0) && (iteration + 1 == maxIter);
const bool use_relaxed_mb = relax_final_iteration || (iteration >= param_.min_strict_mb_iter_);
And then replace the if () below with
if (relax_final_iteration) {
// Write the message about trying with relaxed tolerances in the final iteration.
}
There was a problem hiding this comment.
I have updated accordingly.
|
jenkins build this please |
2 similar comments
|
jenkins build this please |
|
jenkins build this please |
|
Time to merge? |
The relaxed tolerance is for now defaulted to the same as the standard tolerance. The idea is to keep the 1e-6 for the relaxed tolerance while updating target to 1e-7. With 19 as default number of strict iterations, this means that if the simulator reach maximum 20 newton iterations it will accept the results if tol < 1e-6.
related to #5157