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

Add step-87: Tutorial on RemotePointEvaluation #15961

Closed
wants to merge 0 commits into from

Conversation

mschreter
Copy link
Contributor

@mschreter mschreter commented Sep 6, 2023

This PR proposes to introduce a new tutorial (PDF) on the usage of advanced point evaluation functionalities based on Utilities::MPI::RemotePointEvaluation. The motivation for this work comes from our ongoing research on two-phase flows with phase change, where we use those functionalities.

We (@peterrum and me) are looking forward to hearing your thoughts about it!

Copy link
Member

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

Some initial small comments. Missing step-87.cc to review. This is a very, very nice example!

examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/results.dox Outdated Show resolved Hide resolved
examples/step-87/doc/results.dox Outdated Show resolved Hide resolved
examples/step-87/doc/results.dox Outdated Show resolved Hide resolved
examples/step-87/doc/results.dox Outdated Show resolved Hide resolved
include/deal.II/base/mpi_remote_point_evaluation.h Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved

// We output the requested points together with the corresponding
// evaluated solution to a CSV file.
std::cout << " - writing csv file" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Is the underlying goal here to illustrate that there many ways to achieve the same thing, or do you wish to demonstrate that some approaches are more efficient / less time consuming?
I am not sure I understand the full motivation for showing three different ways to achieve the same purpose. Generally, my understanding is that it is a bad thing when you have three ways of doing the same things, it's confusing for a new reader.

examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
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 think this is a great tutorial program. I have comments to some text in the introduction and some algorithm choices (in the unimportant parts), but I think this tutorial can help a lot.

examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/doc/intro.dox Outdated Show resolved Hide resolved
@mschreter
Copy link
Contributor Author

@blaisb @kronbichler first of all, thank you very much for taking the time to review this tutorial. In my opinion, your comments are very helpful in improving the readability of the tutorial and also the practical applicability of the code sections. So far, I have addressed the minor points of your reviews. I am at a conference for the next few days and will work on the major points early next week.

@blaisb
Copy link
Member

blaisb commented Sep 13, 2023

@blaisb @kronbichler first of all, thank you very much for taking the time to review this tutorial. In my opinion, your comments are very helpful in improving the readability of the tutorial and also the practical applicability of the code sections. So far, I have addressed the minor points of your reviews. I am at a conference for the next few days and will work on the major points early next week.

We should be the one thanking you for taking the time to contribute such a cool example. This will be helping a lot of people in the long term. I'll finish reviewing the .cc file today and this way when you address the comments, i'll be ready for a second round of review.

Copy link
Member

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

A few last comments, nothing major. @mschreter just ping me when you want another review I'll be glad to do it

examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
examples/step-87/step-87.cc Outdated Show resolved Hide resolved
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 like this tutorial. We have some work left, but I will approve anyway to express my consent with it, and let others continue the discussion.

@mschreter
Copy link
Contributor Author

@blaisb @kronbichler we addressed your suggestions and would be ready for a second round of reviews :-).

We extracted the changes not specific to this tutorial and related to RemotePointEvaluation in two separate PRs #15995 and #15996. I'll rebase once we are finished with the review.

@peterrum
Copy link
Member

peterrum commented Oct 1, 2023

I accidentally closed this PR when rebasing and fixing merge conflicts. The new PR is #16072.

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