Skip to content

Fix restrict=True for actions#5260

Open
leo-collins wants to merge 4 commits into
releasefrom
leo/restrict-action
Open

Fix restrict=True for actions#5260
leo-collins wants to merge 4 commits into
releasefrom
leo/restrict-action

Conversation

@leo-collins

Copy link
Copy Markdown
Contributor

We need to replace the contracted argument

This exposed a problem in applying BCs to an action
connorjward
connorjward previously approved these changes Jul 16, 2026

@connorjward connorjward 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.

Seems to make sense.

@pbrubeck

Copy link
Copy Markdown
Contributor

What error were you getting? It should be possible still to assemble the original unrestricted F and then interpolate the result into the restricted space. This seems like a workaround

@leo-collins

Copy link
Copy Markdown
Contributor Author

What error were you getting?

An error in UFL for incompatible spaces in action.

It should be possible still to assemble the original unrestricted F and then interpolate the result into the restricted space.

I don't understand. Where are we assembling F?

@pbrubeck

pbrubeck commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

I don't understand. Where are we assembling F?

assemble(interpolate(v_res, F(u->u_res; v)))

will break into assemble(F(u->u_res; v)) into a Cofunction(V.dual()), and then it will interpolate it to V_res.dual(). There is no need to change the internal arguments in F(u_res; v).

My guess is that the error in UFL might be doing the checks wrong.

@pbrubeck

Copy link
Copy Markdown
Contributor

Also another potential source of issues is the fact that action with an interpolate are eagerly simplifying, maybe that is the problem.

Comment thread tests/firedrake/regression/test_restricted_function_space.py
@leo-collins

Copy link
Copy Markdown
Contributor Author

We have F = action(action(M, I), u) - L (looking at the test I added). If we replace(F, {self.u: self.u_restrict}) then we end up with action(action(M, I), u_restricted). action(M, I) goes from V -> V^*, and u_restricted lives in V_restricted. Hence the incompatible spaces.

In compute_bc_lifting we do F = ufl_expr.action(J, u), where J=action(M, I). However this returns an Action(Action(M, I), u) since J is a BaseForm. If J is a regular Form, then ufl_expr.action(J,u) eagerly computes the action and returns another Form.

@pbrubeck

Copy link
Copy Markdown
Contributor

We have F = action(action(M, I), u) - L (looking at the test I added). If we replace(F, {self.u: self.u_restrict}) then we end up with action(action(M, I), u_restricted). action(M, I) goes from V -> V^*, and u_restricted lives in V_restricted. Hence the incompatible spaces.

In compute_bc_lifting we do F = ufl_expr.action(J, u), where J=action(M, I). However this returns an Action(Action(M, I), u) since J is a BaseForm. If J is a regular Form, then ufl_expr.action(J,u) eagerly computes the action and returns another Form.

Sounds like one mathematically safe fix is to replace u -> interpolate(u_res, V)

Comment thread firedrake/variational_solver.py Outdated
self.F = replace(F, {F_arg: v_res, self.u: self.u_restrict})
else:
self.F = interpolate(v_res, replace(F, {self.u: self.u_restrict}))
self.F = interpolate(v_res, replace(F, {u_arg: u_res, self.u: self.u_restrict}))

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.

Suggested change
self.F = interpolate(v_res, replace(F, {u_arg: u_res, self.u: self.u_restrict}))
self.F = interpolate(v_res, replace(F, {self.u: interpolate(self.u_restrict, V)}))

@pbrubeck pbrubeck Jul 16, 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.

There are many ways to "fix" this. Of course my suggestion is far from optimal because we need to generate code and incur extra flops. But I think this is the most robust solution. We can make it efficient if we intercept V_res->V in interpolate and replace the codegen with dof-permutation (i.e. assign)

Comment thread tests/firedrake/regression/test_restricted_function_space.py
Comment on lines +113 to +118
if isinstance(self.J, Form):
v_arg, u_arg = self.J.arguments()
self.J = replace(self.J, {v_arg: v_res, u_arg: u_res, self.u: self.u_restrict})
else:
J_full = replace(self.J, {self.u: u_full})
self.J = ufl_expr.action(Pstar, ufl_expr.action(J_full, P))

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.

Should we turn this into a reusable function? It might be good to generalize to any number of arguments

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.

3 participants