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

correct docu of GridTools::transform() and GridGenerator::cylinder() #10079

Merged
merged 1 commit into from
May 8, 2020

Conversation

nfehn
Copy link
Contributor

@nfehn nfehn commented May 8, 2020

Closes #8920.

Since no changes to the implemented will be made soon, the docu should correctly describe the current behavior.

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.

I have one request regarding the documentation and one question: Shouldn't we simply add triangulation.reset_all_manifolds() inside the GridTools::transform implementation? This is how I understood the consensus in the issue #8914.

The thing is that at least for translations and rotations it should be reasonably simple to put a manifold that applies a transformation for us between the old manifold and the new one, apart from the difficulties mentioned in #8914 (comment). So this behavior might change in the future if someone is willing to implement it, and putting the reset_all_manifolds() inside the library for the cases we don't know the result seems more consistent to me.

Comment on lines 431 to 433
* to the triangulation. If you want to perform refinements according to a
* manifold description, you should first do the refinements, subsequently
* deactivate all manifolds, and finally call the transform() function. The
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this statement on refinements is true in this generality - it is perfectly valid to refine the mesh after the transformation if one has a manifold for the transformed geometry at hand. So I would state this as

Suggested change
* to the triangulation. If you want to perform refinements according to a
* manifold description, you should first do the refinements, subsequently
* deactivate all manifolds, and finally call the transform() function. The
* to the triangulation. If you want to perform refinements according to a
* manifold description, you either need to attach the appropriate manifold
* for the transformed geometry and clear possibly old manifolds, or
* do the refinements before the transformation, deactivate all manifolds,
* and finally call the transform() function. The

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 can incorporate this idea. However, I would reformulate it a bit because the sentence becomes quite complicated now.

@nfehn
Copy link
Contributor Author

nfehn commented May 8, 2020

Regarding your question, I agree principally, but I think we have issue #8914 to discuss these basic decisions and separate PRs to work on solutions. I think here we should focus on the documentation.

@kronbichler kronbichler added this to the Release 9.2 milestone May 8, 2020
@kronbichler kronbichler merged commit e7736d9 into dealii:master May 8, 2020
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.

Documentation of GridGenerator::cylinder() incomplete and incorrect.
2 participants