Skip to content

Update BCS for forward recomputation #4408

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Ig-dolci
Copy link
Contributor

Description

This PR seeks to fix the issue #4387, which is related to the Nonlinear Variational Solve not updating the Dirichlet boundary conditions based on the checkpointed functions.

if u:
r.assign(u - self.function_arg, subset=self.node_set)
r.assign(u - bc, subset=self.node_set)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still missing a test for this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

You will get this case by passing pre_apply_bcs=False to the NLVS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.

@Ig-dolci Ig-dolci changed the title Update bcs for adjoints Update BCS for forward recomputation Jun 26, 2025
Comment on lines +454 to +456
bc_checkpointed = self.block_variable.checkpoint
bc = bc_checkpointed.checkpoint \
if bc_checkpointed is not None else self.function_arg
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this logic be put into a new property that gets overloaded in the corresponding adjoint subclass of DirichletBC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it must be. I am still checking to see if this works. It looks like it requires more thought. Sia just wrote that this is leading some Gadopt tests to fail.

@sghelichkhani
Copy link
Contributor

sghelichkhani commented Jun 27, 2025

I know this is still wip, but flagging that this breaks one of our tests and the unit test here. Our CI is failing as we load the BC from a checkpoint file:

  File "/opt/firedrake/firedrake/bcs.py", line 461, in apply
    r.assign(bc, subset=self.node_set)
  File "/opt/firedrake/firedrake/adjoint_utils/function.py", line 116, in wrapper
    ret = assign(self, other, *args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/firedrake/firedrake/function.py", line 461, in assign
    Assigner(self, expr, subset).assign()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/firedrake/firedrake/assign.py", line 147, in __init__
   expression = as_ufl(expression)
                 ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/dist-packages/ufl/constantvalue.py", line 517, in as_ufl
    raise ValueError(
ValueError: Invalid type conversion: <firedrake.adjoint_utils.checkpointing.DelegatedFunctionCheckpoint object at 0x
73029d9272f0> can not be converted to any UFL type.

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