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

Restricted Cofunction RHS #3922

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

pbrubeck
Copy link
Contributor

@pbrubeck pbrubeck commented Dec 11, 2024

Description

This PR enables solve(a == assemble(L), u, bcs, restrict={False|True}) without requiring users to set the boundary nodes to zero when they provide a Cofunction RHS. Here we also solved a few other issues:

  1. V and V.dual() do no longer compare to the same thing.
  2. assemble(form, bcs, zero_bc_nodes=True) is the new default, and for the False case we throw an error because we will try to apply primal inhomogenous data on a dual Cofunction.

Depends on firedrakeproject/ufl#53

Fixes #3203, #3498, and #3130

Supersedes #3214, #3754

Copy link

github-actions bot commented Dec 11, 2024

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake complex8126 ran6652 passed1474 skipped0 failed

Copy link

github-actions bot commented Dec 11, 2024

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake real8132 ran7457 passed675 skipped0 failed

firedrake/assemble.py Outdated Show resolved Hide resolved
firedrake/assemble.py Outdated Show resolved Hide resolved
Comment on lines 96 to 98
for c in F.coefficients():
if c.function_space() == V.dual():
replace_F[c] = Cofunction(V_res.dual()).interpolate(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you do this, but I would like to think a bit more to see if there is any better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hoped something like self.F = interpolate(F, V_res.dual()) would work, but it looks not supported, so maybe this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a way to make self.F = action(replace(F, {u: u_res}), interpolate(v_res, V)) work. It requires fixing some FormSum bugs, and fixing the way assemble optimizes and applies BCs on BaseForms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

@pbrubeck pbrubeck force-pushed the pbrubeck/fix/restricted-cofunction branch from 628fc95 to 0d70a66 Compare December 13, 2024 18:56
Copy link
Contributor

@ksagiyam ksagiyam left a comment

Choose a reason for hiding this comment

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

Please fix CI.

Comment on lines 96 to 98
for c in F.coefficients():
if c.function_space() == V.dual():
replace_F[c] = Cofunction(V_res.dual()).interpolate(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

I hoped something like self.F = interpolate(F, V_res.dual()) would work, but it looks not supported, so maybe this is fine.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@ksagiyam ksagiyam requested a review from Ig-dolci December 18, 2024 13:22
firedrake/assemble.py Outdated Show resolved Hide resolved
@@ -225,8 +225,12 @@ def assign(self, expr, subset=None, expr_from_assemble=False):
return self.assign(
assembled_expr, subset=subset,
expr_from_assemble=True)

raise ValueError('Cannot assign %s' % expr)
elif expr == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

We do the same for ufl.classes.Zero on line 199 so this should probably be combined. In general I am a bit concerned about this code because we do not tape Assigner. @Ig-dolci knows more I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I copied this from Function.assign, and just noticed that Function.assign is decorated with @FunctionMixin._ad_annotate_assign. This is required for bc.set(Cofunction, float) if we want to set diagonal entries to 1.

firedrake/linear_solver.py Outdated Show resolved Hide resolved
tests/firedrake/regression/test_assemble_baseform.py Outdated Show resolved Hide resolved
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/restricted-cofunction branch 2 times, most recently from 8ee8028 to 34669b7 Compare December 19, 2024 17:17
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/restricted-cofunction branch from 34669b7 to fe30b48 Compare December 19, 2024 17:21
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/restricted-cofunction branch from 0acaa6e to 6fdaf9d Compare December 20, 2024 02:16
firedrake/assemble.py Outdated Show resolved Hide resolved
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/restricted-cofunction branch from 6fdaf9d to df04f4b Compare December 20, 2024 16:47
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/restricted-cofunction branch from 8ada771 to 73be82b Compare December 21, 2024 03:20
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/restricted-cofunction branch from 73be82b to a86f3f5 Compare December 21, 2024 03:21
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.

BUG: LinearSolver.solve fails when passed two Functions.
3 participants