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

Implement ReferenceCell::point_is_inside. #13496

Merged
merged 3 commits into from Mar 9, 2022

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Mar 6, 2022

This is the generalized equivalent of GeometryInfo::is_inside_unit_cell().

/rebuild

@peterrum
Copy link
Member

peterrum commented Mar 6, 2022

FYI @nfehn

*/
template <int dim>
bool
point_is_inside(const Point<dim> &p, const double eps = 0) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe formulate as question is_point_inside().

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like is_point_inside as a name. It sounds grammatically wrong.

Would contains_point be better? As in

  cell->contains_point(p)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just is_inside(), in analogy to std::is_same, std::is_enum, etc.

(point.is_inside(cell) would be easier to read, but does not make sense from a software design perspective. I assume this is the reason why you repeated „point“ in the function name?)

/**
* Return true if the given point is inside the reference cell of the present
* space dimension. This function accepts an additional parameter (which
* defaults to zero) which
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe such geometry functions with tolerances should not use default parameters.

I think the documentation does not exactly specify what eps = 0 means. I would understand the docu as follows: A point lying on the boundary together with eps=0 will result in a return value of true. However, the formulation in the first sentence "if the given point is inside the reference cell ..." goes in a different direction.

I would prefer tolerance (often used in GridTools) over eps. Is it really numerical roundoff in the sense of eps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made those changes.

@bangerth
Copy link
Member Author

bangerth commented Mar 7, 2022

What do others think about the naming of the function?

@peterrum
Copy link
Member

peterrum commented Mar 8, 2022

What do others think about the naming of the function?

I am fine with contains_point(). One would read cell.contains_point(a) as cell containts point a. If one wants to be shorter, one could also write contains(). But nut sure if that becomes ambiguous.

@nfehn
Copy link
Contributor

nfehn commented Mar 8, 2022

I understand "contain" in the sense of information/data, not geometrically.

@nfehn
Copy link
Contributor

nfehn commented Mar 8, 2022

Assume you want to know whether a cell (of one triangulation) is enclosed by another triangulation tria. The example tria->contains_cell(cell) in my opinion clearly demonstrates that the "contains" nomenclature is misleading. Put differently, we should find a wording that is intuitive not only for the cell/point example, but also for tria/cell and other examples.

@Rombur
Copy link
Member

Rombur commented Mar 8, 2022

I don't have a strong opinion on the name. If we want something more general, we could use the same naming convention as boost. Use within if a box is totally inside another box and intersect if a boxes is totally or partially inside an other one. So for this function, something like point_within_cell.

@nfehn
Copy link
Contributor

nfehn commented Mar 8, 2022

cell.point_within_cell(point) might be quite hard to read (then, we would need a free function like point_within_cell(point,cell) in my opinion).

What about cell.encloses(point) as one more suggestion ;)

@bangerth
Copy link
Member Author

bangerth commented Mar 8, 2022 via email

@masterleinad
Copy link
Member

I like contains_point.

This is the generalized equivalent of GeometryInfo::is_inside_unit_cell().
@bangerth
Copy link
Member Author

bangerth commented Mar 9, 2022

contains_point() it is. I like that name better than point_is_inside() anyway :-)

So updated!

@peterrum peterrum merged commit df6305f into dealii:master Mar 9, 2022
@bangerth bangerth deleted the point_is_inside branch March 10, 2022 04:37
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

5 participants