Skip to content

Parallelize periodic RVE boundary conditions - #2102

Open
m-frey wants to merge 4 commits into
4C-multiphysics:mainfrom
m-frey:dev-pbc-parallel
Open

Parallelize periodic RVE boundary conditions#2102
m-frey wants to merge 4 commits into
4C-multiphysics:mainfrom
m-frey:dev-pbc-parallel

Conversation

@m-frey

@m-frey m-frey commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Description and Context

The periodic RVE boundary conditions in the constraint framework were
serial-only and aborted with a FOUR_C_THROW on more than one rank. This
enables them in parallel.

Changes:

  • Match periodic node pairs with a distributed ArborX search
    (global_collision_search) instead of a rank-local BVH query.
  • Ghost the matched partner and corner nodes onto the owning rank so all
    four nodes of a constraint are locally available.
  • Assemble each constraint row on a single owner; derive Q_dL
    and the residual from a distributed mat-vec, so no off-processor
    assembly is needed.

Related Issues and Pull Requests

@m-frey
m-frey requested review from isteinbrecher and mayrmt June 29, 2026 16:09
@m-frey m-frey self-assigned this Jun 29, 2026
Copilot AI review requested due to automatic review settings June 29, 2026 16:09
Copilot AI reviewed Jun 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

georghammerl
georghammerl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
mayrmt
mayrmt reviewed Jul 7, 2026
View reviewed changes
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_base.hpp Outdated
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
bennoschoenstein
bennoschoenstein previously requested changes Jul 16, 2026
View reviewed changes

@bennoschoenstein bennoschoenstein left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks a lot! Exactly what I need 🗡️

Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp
Comment thread src/constraint_framework/4C_constraint_framework_equation.cpp Outdated
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
@mayrmt

mayrmt commented Sep 2, 2026

Copy link
Copy Markdown
Member

@m-frey is there anything that is blocking this PR to move forward?

@bennoschoenstein
bennoschoenstein force-pushed the dev-pbc-parallel branch 2 times, most recently from 2015e4c to fadcf44 Compare September 29, 2026 11:27
mayrmt
mayrmt reviewed Sep 29, 2026
View reviewed changes
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
@bennoschoenstein
bennoschoenstein dismissed their stale review October 5, 2026 12:34

Adressed the issues.

@bennoschoenstein

bennoschoenstein commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

@isteinbrecher could you take a look at this implementation?

@georghammerl
georghammerl requested a balanced review from Copilot October 5, 2026 13:49
Copilot AI reviewed Oct 5, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Replicating all positive-boundary DOF metadata on every rank creates a significant scalability bottleneck.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp
georghammerl
georghammerl previously approved these changes Oct 5, 2026
View reviewed changes
isteinbrecher
isteinbrecher reviewed Oct 5, 2026
View reviewed changes
Comment thread src/constraint_framework/4C_constraint_framework_equation.cpp
constraint_vector.sum_into_global_values(1, &constraintViolation, &rowId);
FOUR_C_ASSERT(Q_Ld.row_map().my_gid(row_id),
"Constraint equation row {} is not owned by this rank.", row_id);
Q_Ld.assemble(coefficient, row_id, dof_id);

@isteinbrecher isteinbrecher Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is Q_dL not here any more?

@bennoschoenstein bennoschoenstein Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I removed it because in parallel the equation would have to write into rows that belong to another rank. I will put it back and make sure those entries reach the rank that owns the row.

Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_base.cpp Outdated
Q_dL_->complete(*n_condition_map_, stiff_ptr_->domain_map());

// Q_dL = Q_Ld^T
Q_dL_ = Core::LinAlg::matrix_transpose(*Q_Ld_);

@isteinbrecher isteinbrecher Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I get why you do this, but this is a change we don't want, as it restricts the design of the constraint evaluator. In general, a constraint can assemble into Q_dL_ and Q_Ld and the terms can be different.

No case covers this as of now, but we should not narrow the possible applications.

@bennoschoenstein bennoschoenstein Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed. I will remove the transpose here, so the two can differ.

Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
Comment on lines +80 to +106
// Turns floating point exceptions off while this object exists and restores them afterwards.
// Needed for the ArborX search: a rank without local boundary nodes is empty, which triggers a
// harmless 0/0.
class SuspendFloatingPointTrapping
{
public:
SuspendFloatingPointTrapping()
{
#ifdef FOUR_C_ENABLE_FE_TRAPPING
fedisableexcept(FE_INVALID | FE_DIVBYZERO | FE_OVERFLOW);
#endif
}
~SuspendFloatingPointTrapping()
{
#ifdef FOUR_C_ENABLE_FE_TRAPPING
feclearexcept(FE_ALL_EXCEPT);
fedisableexcept(FE_ALL_EXCEPT);
feenableexcept(previously_enabled_excepts_);
#endif
}
SuspendFloatingPointTrapping(const SuspendFloatingPointTrapping&) = delete;
SuspendFloatingPointTrapping& operator=(const SuspendFloatingPointTrapping&) = delete;

#ifdef FOUR_C_ENABLE_FE_TRAPPING
private:
const int previously_enabled_excepts_ = fegetexcept();
#endif

@isteinbrecher isteinbrecher Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems very fishy to me. Why not use our ArborX interface and if it does not cover this case, adapt the interface. This will be much cleaner than to fiddle with the floating point exceptions here.

@maxfirmbach maxfirmbach Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have to agree with @isteinbrecher. This piece of code should not make it into main. It does not fix the actual problem.

@bennoschoenstein bennoschoenstein Oct 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done.
The fix is now in global_collision_search: a rank without primitives passes one dummy primitive, and its hits are removed from the results.
The guard in the constraint code is gone.

Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp
@@ -150,9 +195,6 @@ Constraints::SubmodelEvaluator::RveMultiPointConstraintManager::RveMultiPointCon
*----------------------------------------------------------------------------*/
void Constraints::SubmodelEvaluator::RveMultiPointConstraintManager::check_input()

@isteinbrecher isteinbrecher Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The diff in this function is not easily readable. Can you please explain why there are so many changes needed here?

@bennoschoenstein bennoschoenstein Oct 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I added comments at the specific locations. Let me know if that clarifies things.

bennoschoenstein
bennoschoenstein reviewed Oct 6, 2026
View reviewed changes
Comment thread src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp Outdated
@bennoschoenstein
bennoschoenstein requested review from georghammerl, isteinbrecher and maxfirmbach and a balanced review from Copilot October 6, 2026 12:48
Copilot AI reviewed Oct 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@bennoschoenstein
bennoschoenstein requested a balanced review from Copilot October 12, 2026 14:32
Copilot AI reviewed Oct 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@maxfirmbach

maxfirmbach commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@bennoschoenstein Can you please create an issue and describe the problem we encounter here? Thanks.

@bennoschoenstein bennoschoenstein mentioned this pull request Oct 14, 2026
Closed
m-frey and others added 4 commits October 22, 2026 17:34
@m-frey @bennoschoenstein
Parallelize periodic RVE boundary conditions
8c75913 
Distributed pair search, one owner per constraint, dof lookup instead
of ghosting.
@bennoschoenstein
Improve comments
1245226
@bennoschoenstein
Assemble Q_dL in the constraint equation again
57efe9b 
Q_dL_ is an FE matrix now, so entries for dof rows owned by another
rank are forwarded on complete() instead of deriving Q_dL as the
transpose of Q_Ld.
@bennoschoenstein
Remove floating point guard around periodic search
b0970ad 
ArborX handles ranks without primitives in the distributed tree since
kokkos/ArborX#1404 (4C 4C-multiphysics#2353), which closes 4C-multiphysics#2340.
@bennoschoenstein

bennoschoenstein commented Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Rebased on current main and removed the dummy-primitive workaround, since #2353 fixes #2340.
@maxfirmbach @isteinbrecher

@isteinbrecher

isteinbrecher commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Rebased on current main and removed the dummy-primitive workaround, since #2353 fixes #2340.
@maxfirmbach @isteinbrecher

Did this fix your problem with the search?

@bennoschoenstein

bennoschoenstein commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

@isteinbrecher it fixed the problem described in #2340 when a rank is "empty". Is that what you mean with "search"?

@maxfirmbach

maxfirmbach commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

@bennoschoenstein Yep, thats what he means.

@mayrmt

mayrmt commented Oct 23, 2026

Copy link
Copy Markdown
Member

@bennoschoenstein What is require to move forward with this PR (except for code review & approvals)?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@bennoschoenstein bennoschoenstein bennoschoenstein left review comments
@sbrandstaeter sbrandstaeter Awaiting requested review from sbrandstaeter
@mayrmt mayrmt Awaiting requested review from mayrmt
@isteinbrecher isteinbrecher Awaiting requested review from isteinbrecher
@maxfirmbach maxfirmbach Awaiting requested review from maxfirmbach
@georghammerl georghammerl Awaiting requested review from georghammerl

At least 2 approving reviews are required to merge this pull request.

Assignees

@m-frey m-frey

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@m-frey @mayrmt @bennoschoenstein @maxfirmbach @isteinbrecher @georghammerl