Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Better document what tolerance to use in MappingCartesian. #14103

Merged
merged 1 commit into from
Jul 9, 2022
Merged

Better document what tolerance to use in MappingCartesian. #14103

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 16 additions & 7 deletions source/fe/mapping_cartesian.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,28 @@ is_cartesian(const CellType &cell)
if (!cell->reference_cell().is_hyper_cube())
return false;

const double tolerance = 1e-14;
const double abs_tol = 1e-15;
const double rel_tol = 1e-14;
const auto bounding_box = cell->bounding_box();
const auto & bounding_vertices = bounding_box.get_boundary_points();
const auto cell_measure =
const auto bb_diagonal_length_squared =
bounding_vertices.first.distance_square(bounding_vertices.second);

for (const unsigned int v : cell->vertex_indices())
{
const double vertex_tolerance =
tolerance * std::max(cell->vertex(v).norm_square(), cell_measure);

if (cell->vertex(v).distance_square(bounding_box.vertex(v)) >
vertex_tolerance)
// Choose a tolerance that takes into account both that vertices far
// away from the origin have only a finite number of digits
// that are considered correct (an "absolute tolerance"), as well as that
Comment on lines +68 to +69
Copy link
Member

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.

// vertices are supposed to be close to the corresponding vertices of the
// bounding box (a tolerance that is "relative" to the size of the cell).
//
// We need to do it this way because when a vertex is far away from
// the origin, computing the difference between two vertices is subject
// to cancellation.
const double tolerance = std::max(abs_tol * cell->vertex(v).norm_square(),
rel_tol * bb_diagonal_length_squared);
Comment on lines +76 to +77
Copy link
Member

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?


if (cell->vertex(v).distance_square(bounding_box.vertex(v)) > tolerance)
return false;
}

Expand Down