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

Ghosting #27810

Open
wants to merge 19 commits into
base: next
Choose a base branch
from
Open

Ghosting #27810

wants to merge 19 commits into from

Conversation

oanaoana
Copy link
Contributor

@oanaoana oanaoana commented Jun 6, 2024

This PR addresses #27203.

@moosebuild
Copy link
Contributor

moosebuild commented Jun 6, 2024

Job Documentation on 3b8c7dd wanted to post the following:

View the site here

This comment will be updated on new commits.

@vincentlaboure
Copy link
Contributor

As discussed with @oanaoana, the proposed test already passes with the current version of MOOSE (without her changes) so a better test should be conceived to be fully convincing. Ideally such a test would show a memory spike during mesh initialization before the change but not after.

@moosebuild
Copy link
Contributor

moosebuild commented Jun 10, 2024

Job Coverage on 3b8c7dd wanted to post the following:

Framework coverage

62dc98 #27810 3b8c7d
Total Total +/- New
Rate 85.06% 85.03% -0.03% 12.50%
Hits 104412 104417 +5 6
Misses 18335 18378 +43 42

Diff coverage report

Full coverage report

Modules coverage

Heat transfer

62dc98 #27810 3b8c7d
Total Total +/- New
Rate 88.95% 88.99% +0.04% 100.00%
Hits 4258 4275 +17 18
Misses 529 529 - 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 12.50% is less than the suggested 90.0%

This comment will be updated on new commits.

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

it would be good to create a framework test to try and improve coverage

framework/doc/content/syntax/Mesh/splitting.md Outdated Show resolved Hide resolved
framework/doc/content/syntax/Mesh/splitting.md Outdated Show resolved Hide resolved
framework/include/relationshipmanagers/GhostBoundary.h Outdated Show resolved Hide resolved
framework/include/relationshipmanagers/GhostBoundary.h Outdated Show resolved Hide resolved
framework/src/relationshipmanagers/GhostBoundary.C Outdated Show resolved Hide resolved
framework/src/relationshipmanagers/GhostBoundary.C Outdated Show resolved Hide resolved
framework/src/relationshipmanagers/GhostBoundary.C Outdated Show resolved Hide resolved
framework/src/relationshipmanagers/GhostBoundary.C Outdated Show resolved Hide resolved
for (auto side : elem->side_index_range())
for (auto boundary_id : boundary_ids)
if ((elem->processor_id() != p) && (binfo.has_boundary_id(elem, side, boundary_id)))
coupled_elements.insert(std::make_pair(elem, _null_mat));
Copy link
Member

Choose a reason for hiding this comment

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

At the point that you've identified the element as needing to be ghosted, you can break out of the boundary id and side loops

Copy link
Member

Choose a reason for hiding this comment

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

Breaking out of two loops the easiest way requires a goto statement, which is arguably as much bad as it is good. I'm going to suggest that we simply create "a good first" issue to revisit this since you've pointed it out in this review, but I wouldn't necessarily hold up the merge of this PR for this single optimization.

Copy link
Member

Choose a reason for hiding this comment

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

People talk about goto being bad, but I think it's a dogma. goto can be used very effectively and clearly and would be a very reasonable two line addition here to save potentially O(n sides) * O(n boundary IDs) unnecessary looping

Copy link
Member

Choose a reason for hiding this comment

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

the amount of time required to implement this change should be under two minutes

@moosebuild
Copy link
Contributor

Job Precheck on 912a1bc wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/27810/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 95d6f4fd6c8286879312d2633ce3f0af5de48949

Comment on lines +102 to +104
goto countBreak;
countBreak:
coupled_elements.insert(std::make_pair(elem, _null_mat));
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
goto countBreak;
countBreak:
coupled_elements.insert(std::make_pair(elem, _null_mat));
{
coupled_elements.insert(std::make_pair(elem, _null_mat));
goto countBreak;
}
countBreak:

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

Successfully merging this pull request may close these issues.

None yet

5 participants