-
Notifications
You must be signed in to change notification settings - Fork 707
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
Fix references to Boundary #5924
Fix references to Boundary #5924
Conversation
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.
Thanks for taking the time to do this. I think that we should take this opportunity improve some phrasing while we revise this part of the documentation.
@@ -147,7 +147,7 @@ asterisks. It is worth pointing out one other thing here, though: because we | |||
detach the manifold description from the surface mesh, whenever we use a | |||
mapping object in the rest of the program, it has no curves boundary | |||
description to go on any more. Rather, it will have to use the implicit, | |||
StraightBoundary class that is used on all parts of the boundary not |
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.
While we are here: it is more correct to write 'on all parts of the domain not explicitly assigned a different manifold object' since manifolds are a conceptually orthogonal concept to boundary descriptions.
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.
Fixed.
@@ -66,8 +66,8 @@ the OpenCASCADE framework. From a <code>TopoDS_Shape</code>, it is then | |||
possible to access all the sub-shapes (such as vertices, edges and faces) | |||
composing it, along with their geometrical description. In the deal.II | |||
framework, the topological entities composing a shape are used to create | |||
objects of the Manifold or Boundary classes. In Step-6 we saw how to build a |
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.
I read the sentence
the topological entities composing a shape are used to create objects of the Manifold classes.
several times and I honestly have no idea what this means. Could we reword this sentence?
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.
I only changed a few words. Does this sound better to you?
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.
I think so, but I honestly still don't understand it. What does 'topological entities' mean here?
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.
"topological entities" is what OpenCASCADE uses to define an abstract geometrical object, or a shape (a Topo_DS_Shape
). Shapes can be vertices, edges, faces, volumes, and any combination of the above. We have no such a thing in deal.II, but OpenCASCADE
does, and this documentation refers somehow to this nomenclature.
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.
In OpenCASCADE a TopoDSShape
is a collection of topological shapes. Some of our manifolds work with faces, other with edges, but all of them can be encapsulated in a OpenCASCADE Shape
, which is the entry that is returned, for example, when opening IGES files.
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.
Yes, this makes sense now. Thanks.
examples/step-54/doc/intro.dox
Outdated
objects of the Manifold or Boundary classes. In Step-6 we saw how to build a | ||
HyperBallBoundary and assign it to a set of faces (or cells, for co-dimension | ||
objects of the Manifold classes. In Step-6 we saw how to build a | ||
SphericalManifold and assign it to a set of faces (or cells, for co-dimension | ||
1) of a Triangulation, to have cells and faces refined on a sphere or circle. | ||
The functions of the CAD modeling interface have been designed to retain the | ||
same structure, allowing the user to build a projector object using the |
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.
line 75 references HyperBallBoundary
examples/step-54/doc/intro.dox
Outdated
@@ -79,7 +79,7 @@ onto the specified geometry. | |||
|
|||
Differently from a spherical or circular boundary, a boundary with a complex | |||
geometry poses problems as to where it is best to place the new nodes created | |||
upon refinement on the prescribed shape. HyperBallBoundary first creates the | |||
upon refinement on the prescribed shape. SphericalManifold first creates the | |||
new nodes on the face or edge to be refined by averaging the surrounding | |||
points in the same way as FlatManifold does. Then, it goes on to project such |
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.
new nodes on the face or edge to be refined by averaging the surrounding points in the same way as FlatManifold does.
I don't think that this is what SphericalManifold
does, but I might be wrong (I think the averaging is not the same as what is done by FlatManifold
).
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.
You are totally right. I switched to PolarManifold
which is much easier to describe.
@@ -1130,7 +1130,7 @@ namespace GridGenerator | |||
* assign appropriate boundary objects through Triangulation::set_boundary() |
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.
We should not mention set_boundary
any more.
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.
I assume we have to fix that other places as well.
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.
true; perhaps we can leave this for another PR.
b78ee32
to
c0c70aa
Compare
examples/step-54/doc/intro.dox
Outdated
upon refinement on the prescribed shape. PolarManifold, for example, transforms | ||
the surrounding points to polar coordinates, calculates the average in that | ||
coordinate system (for each coordinate individually) and finally transforms back | ||
the point to Cartesian coordinates. |
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.
should be 'transforms the point back', but this is much easier to read now 👍
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.
I fixed the same phrasing in include/deal.II/grid/manifold_lib.h
as well.
c0c70aa
to
9d4e696
Compare
@drwells I fixed the last remaining comment and squashed. |
Yup, this looks good to me. Thank you for dealing with all of my pedantic comments. |
Relates to #4800. This PR replaces the remaining references to
Boundary
I could find (that are not in the corresponding header or source files). Inmapping_q_generic.cc
we also check explicitly for the boundary description to be aStraightBoundary
so this has to stay as well until we remove the class.