New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better document what tolerance to use in MappingCartesian. #14103
Conversation
// away from the origin have only a finite number of digits | ||
// that are considered correct (an "absolute tolerance"), as well as that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding the term "cancellation" here, as this is exactly what happens: Far away from the origin, the difference between two vertices is subject to cancellation according to this quantity.
const double tolerance = std::max(abs_tol * cell->vertex(v).norm_square(), | ||
rel_tol * bb_diagonal_length_squared); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not your fault but already present in @gassmoeller's fix: I'm wondering whether this is the correct magnitude for the tolerances. Shouldn't we use something like abs_tol=1e-30
and rel_tol=1e-28
(or maybe a bit weaker), since we are comparing the square of small numbers, so we should target something slightly above the square of the machine epsilon? Did you check the numbers that we actually get?
@kronbichler I've updated the comment to mention the issue with cancellation. As for the magnitude of the tolerance: I think this is correct. We are comparing quantities here in floating point arithmetic. Whether these quantities happen to be the squares of something or not is not important, and so I believe that the tolerances should be on the order of 1e-16 and not 1e-32. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's get this in, thank you.
/rebuild |
This is a slight change in functionality as well, so we should cherry pick for 9.4.1. |
Anyone have their 9.4 branch still sitting around? |
Better document what tolerance to use in MappingCartesian.
A follow-up to #14092.
/rebuild