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

ParMETIS on simple domains correction #27691

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

oanaoana
Copy link
Contributor

This PR closes #25691.

@moosebuild
Copy link
Contributor

Job Precheck on 26257af 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/27691/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 ef92d30475a0cdfae7cd039c98567b33e4f620c0

@@ -1,6 +1,8 @@
# PetscExternalPartitioner

Allow users to use several external partitioning packages (parmetis, chaco, ptscotch and party) via PETSc.
Note that partitioning, just as meshing, requires a level of user insight to assure the domain is appropriately distributed. For example, edge cases where one seeks to have very few elements per process
may misbehave with certain partitioners. To avert such situations, we switch ParMETIS to PTScotch in cases with less than 28 elements per process, and notify the user with a warning.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's hard to come up with a good answer to "at what point should we overrule the user and stop trusting Parmetis", but the answer in the docs is 28, the answer in the comment is 32, and the answer in the code is 20? :-D

I don't see any other problems here, but I'll wait until after your planned squash/rebase/etc before submitting an official review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! With the going back and forth on the figs and doc I didn't get to make sure it's consistent everywhere. I would have noticed after civet runs and I give it another look. Annoying one can't run all the civet tests before pushing, the local tests aren't always enough.

@moosebuild
Copy link
Contributor

moosebuild commented May 22, 2024

Job Documentation on ada3938 wanted to post the following:

View the site here

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.

So it sounded like you guys were able to reproduce the poor performance of parmetis with small local element counts without PetscExternalPartitioner?

@oanaoana
Copy link
Contributor Author

@lindsayad yeah, using the libmesh one for example. The behaviour is not identical and depends on how the partitioner is initialized. There is certainly a difference between Libmesh and Petsc but the partitioning becomes poor at low elements per process in either case and almost for any initialization. So it made most sense to do a switch for the edge cases.

@NamjaeChoi
Copy link
Contributor

@YaqiWang @jthano

@roystgnr
Copy link
Contributor

partitioning becomes poor at low elements per process in either case

Well, the trick was that in the libMesh case the partitioning becomes better at very low elements per process ... because it turns out that we specifically test for that and switch from Parmetis to serialization+Metis. We don't test robustly enough, so the problem is still reproduceable in some cases, but according to the git logs I hit even worse cases (crashes, not just bad partitioning) back in 2012 and added this same sort of workaround then.

@moosebuild
Copy link
Contributor

Job Coverage on 1a91538 wanted to post the following:

Framework coverage

843ddc #27691 1a9153
Total Total +/- New
Rate 85.09% 85.09% +0.00% 100.00%
Hits 104028 104032 +4 4
Misses 18226 18226 - 0

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

Job Coverage on 1a91538 wanted to post the following:

The following coverage requirement(s) failed:

  • Failed to generate functional_expansion_tools coverage rate (required: 81.0%)

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.

ParMETIS called through MOOSE may deliver broken partitioning
5 participants