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.

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

Assert that the incoming cell is Cartesian in MappingCartesian #13748

Merged
merged 1 commit into from May 17, 2022

Conversation

simonsticko
Copy link
Contributor

No description provided.

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I have a few suggestions for improvement below.



/**
* Return whether the incoming cell is a Cartesian. This is determined by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Return whether the incoming cell is a Cartesian. This is determined by
* Return whether the incoming cell is of Cartesian shape. This is determined by

Comment on lines 56 to 61
for (unsigned int v : cell->vertex_indices())
{
const double tolerance = 1e-14;
if (cell->vertex(v).distance(bounding_box.vertex(v)) > tolerance)
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat check - I would have done it differently but this is certainly the most elegant way to check this property. 👍

const auto bounding_box = cell->bounding_box();
for (unsigned int v : cell->vertex_indices())
{
const double tolerance = 1e-14;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to use relative tolerances:

Suggested change
const double tolerance = 1e-14;
const double tolerance = 1e-14 * cell->diameter();

(I note that this call is somewhat expensive, but this is only used for assertions anyway so we can afford it.)

@kronbichler
Copy link
Member

/rebuild

is_cartesian(const CellType &cell)
{
const auto bounding_box = cell->bounding_box();
for (unsigned int v : cell->vertex_indices())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (unsigned int v : cell->vertex_indices())
for (const unsigned int v : cell->vertex_indices())

template <class CellType>
bool
is_cartesian(const CellType &cell)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would first check if the cell is a hypercube.

bool
is_cartesian(const CellType &cell)
{
const auto bounding_box = cell->bounding_box();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sure if going via bb is right way. Wouldn't it be easier to go through each direction and compare the points?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I can't figure out something easier right now, at least. I think I would just end up with something very similar to what is already in the bounding_box() function:

https://www.dealii.org/developer/doxygen/deal.II/tria__accessor_8cc_source.html#l01616

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it like this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants