Skip to content

Conversation

pbrubeck
Copy link
Contributor

@pbrubeck pbrubeck commented Oct 5, 2025

Description

Fixes a parallel bug when reusing a matfree adjoint Interpolator. Summation with access=INC skipped the halo exchange after the first call to Interpolator.assemble(). On subsequent calls the summation only happened locally within each process, but not across processes.

This bug did not affect assemble(interpolate(...)) because the Interpolator there is never reused.

@pbrubeck pbrubeck force-pushed the pbrubeck/remove-weight-parloop branch from df3e169 to ed3f5b9 Compare October 6, 2025 09:56
@pbrubeck pbrubeck changed the base branch from main to release October 6, 2025 12:57
@pbrubeck pbrubeck changed the title Remove DOF multiplicity parloop Remove DOF multiplicity parloop and fix reuse of matfree adjoint Interpolator Oct 6, 2025
@pbrubeck pbrubeck force-pushed the pbrubeck/remove-weight-parloop branch from ed3f5b9 to 462f355 Compare October 6, 2025 13:17
@connorjward
Copy link
Contributor

Is there any chance this could be two PRs? It's hard to understand the parallel bits when mixed into the other changes.

Also note that the halo freezing stuff is done automatically in pyop3, so maybe you don't have to worry about optimising that here.

@pbrubeck pbrubeck force-pushed the pbrubeck/remove-weight-parloop branch 4 times, most recently from d8d6687 to 19fa12e Compare October 6, 2025 13:38
@pbrubeck
Copy link
Contributor Author

pbrubeck commented Oct 6, 2025

Is there any chance this could be two PRs? It's hard to understand the parallel bits when mixed into the other changes.

I'm having a hard time with git. The parloop removal is the only other thing that's not adressing the bug, should be easy to ignore from the rest of the PR.

@pbrubeck pbrubeck force-pushed the pbrubeck/remove-weight-parloop branch 4 times, most recently from 347184c to 7d4b07a Compare October 7, 2025 07:22
@pbrubeck pbrubeck force-pushed the pbrubeck/remove-weight-parloop branch from 7d4b07a to f7dbe9b Compare October 7, 2025 07:25
v = firedrake.Function(W)
expr = expr._ufl_expr_reconstruct_(operand, v=v)
with weight.dat.vec_ro as w, dual_arg.dat.vec_ro as x, v.dat.vec_wo as y:
callables += (partial(y.pointwiseMult, x, w),)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@connorjward it turns out that this pointwiseMult is the source of the evil. I fixed it by splitting it in two separate calls, so that multiplication is done in-place.

x.copy(y)
y.pointwiseMult(y, w)

I have no explanation why this approach works and the original doesn't.

@pbrubeck pbrubeck force-pushed the pbrubeck/remove-weight-parloop branch from 800bf4c to 21c9173 Compare October 7, 2025 11:16
@pbrubeck pbrubeck force-pushed the pbrubeck/remove-weight-parloop branch from 948c50e to 1a35688 Compare October 7, 2025 11:40
@pbrubeck pbrubeck requested a review from connorjward October 7, 2025 12:44
@pbrubeck pbrubeck changed the title Remove DOF multiplicity parloop and fix reuse of matfree adjoint Interpolator BUG: fix reuse of matfree adjoint Interpolator Oct 9, 2025
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.

2 participants