Fix bugs in AMR TruncationError criterion #6615
Conversation
| umax * rel_truncation_error_coarsened; | ||
| if ((target_rel_truncation_error.has_value() and | ||
| rel_truncation_error_coarsened <= | ||
| target_rel_truncation_error.value()) and |
There was a problem hiding this comment.
I don't generally disagree with people needing to set both, but just for a record of it: would the but be fixed by doing value_or(std::numeric_limits<double>::inf()?
There was a problem hiding this comment.
Yes. Infinity before the second commit changed to zero after would also have worked.
| if (rel_truncation_error_coarsened <= target_rel_truncation_error or | ||
| abs_truncation_error_coarsened <= target_abs_truncation_error) { |
There was a problem hiding this comment.
Could we recast this and the above check into something like:
err = abs(error)/(atol + rtol * abs(u))
I ask because I find that much easier to understand than the logic here, and it nicely maps onto 17.2.7-17.2.9 in Numerical Recipes. I think this does effectively that, if I understand correctly.
Up to you, just an idea on how to eliminate the logic, because I think the logic is subtle and requires more thought.
|
@nilsvu has said he may disagree with this change, and has asked that we wait until he has a change to look at it next week. (And has pointed out that the docs need updating.) |
|
Thanks @wthrowe. No I don't disagree with the change, you can go ahead. When I wrote this logic I did intend to have the criterion satisfy both rel and abs target if both are specified, with the idea that you want to stay below those targets if you set them. This is also what the option helpstrings say, so I don't consider this a bugfix. You're changing the behavior to satisfy either the rel or abs target. If you think that makes more sense then go ahead with the change, but also change the docs. |
|
Okay, so my (rather strong) preference is to use |
|
Yep I agree that it makes more sense to transition between rel and abs tol for this criterion, rather than satisfy both. |
|
Added a fixup and a new commit changing the interface to the power monitor error because it was confusing. |
Setting either value to None completely prevented decreasing resolution.
Previously tried to satisfy both tolerances instead of just the less restrictive.
There are two overloads, one of which returned the error while the other returned -log10 of the error. This was very confusing, and all actual uses immediately undid the logarithm.
Proposed changes
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments