Default to pre_apply_bcs=False#5185
Conversation
pbrubeck
force-pushed
the
pbrubeck/default-pre-apply-bcs
branch
from
51dda7d to
5631a68
Compare
| Ftrial = problem.compute_bc_lifting(self.J, trial, L=problem.L) | ||
| self.F = ufl.replace(Ftrial, {trial: self._x - self._bc_residual}) | ||
| else: | ||
| self.F -= problem.compute_bc_lifting(self.J, self._bc_residual) |
There was a problem hiding this comment.
With pre_apply_bcs=False, we are adding this extra term to the residual F (more code to compile and execute). However this can be prevented in the linear case, as I explain in the comment.
For the nonlinear case, there is a way to reduce execution time by assuming that bc_residual is only supported on cells that along the boundary. We could restrict the integrals in J onto that subset of cells.
There was a problem hiding this comment.
Note that pre_apply_bcs=True implies that self._bc_residual = 0 throughout the solve (if the Newton updates satisfy homogenous BCs by solving the bc identity matrix exactly), and this is why we did not have this term before.
There was a problem hiding this comment.
We could probably lazily assemble this individual term only when bc_residual is nonzero.
pbrubeck
force-pushed
the
pbrubeck/default-pre-apply-bcs
branch
2 times, most recently
from
8e15f0b to
83292ee
Compare
| self.F = ufl.replace(self.F, {self._x: ufl.zero(self._x.ufl_shape)}) | ||
|
|
||
| self.F -= problem.compute_bc_lifting(self.J, self._bc_residual) | ||
| if problem.is_linear and hasattr(problem, "L"): |
There was a problem hiding this comment.
Ideally we would be testing isinstance(problem, LinearVariationProblem), but by doing this I am avoiding a cyclic import. I can have a go at fixing the cyclic import if that is preferred.
pbrubeck
force-pushed
the
pbrubeck/default-pre-apply-bcs
branch
from
83292ee to
7f7a302
Compare
| if bcs and any(isinstance(bc, EquationBC) for bc in bcs): | ||
| restrict = False | ||
| self.restrict = restrict and bcs | ||
| self.restrict = restrict and bool(bcs) |
There was a problem hiding this comment.
This was setting self.restrict = bcs
There was a problem hiding this comment.
I am casting bool(bcs) because the default is bcs=None, and it's too early to extract_bcs(bcs), which parses bcs into an iterable
| transfer_manager | ||
| Object that can transfer functions between levels, | ||
| typically a :class:`~.TransferManager`. | ||
| pre_apply_bcs |
There was a problem hiding this comment.
To unify the codepaths, _SNESContext will no longer branch on pre_apply_bcs and only branch on restrict.
| problem.u_restrict.assign(problem.u) | ||
|
|
||
| if self._ctx.pre_apply_bcs: | ||
| if self.pre_apply_bcs or problem.restrict: |
There was a problem hiding this comment.
In the deprecation era, pre_apply_bcs=True only applies bcs at the beginning of the solve, and _SNESContext becomes independent of pre_apply_bcs, which means that the residual lifting term is always computed (it will be zero when the bcs are satisfied)
1 participant
Description
This PR changes the default model for DirichletBC to
pre_apply_bcs=False, deprecatingpre_apply_bcs=True.In the deprecation era,
pre_apply_bcs=Trueonly applies bcs at the beginning of the solve, and_SNESContextbecomes independent ofpre_apply_bcs, which means that the residual lifting term is always computed (it will be zero when the bcs are satisfied).We also make sure that bcs are always pre applied if
restrict=True, and for this case we never compute the residual lifting term.