Skip to content

Finsberg/interpolate expression - #65

Open
finsberg wants to merge 6 commits into
dokken/interpolate_expressionfrom
finsberg/interpolate_expression
Open

Finsberg/interpolate expression#65
finsberg wants to merge 6 commits into
dokken/interpolate_expressionfrom
finsberg/interpolate_expression

Conversation

@finsberg

Copy link
Copy Markdown
Member

Summary

Merged in finsberg/interpolate_func_into_space to pick up its interpolation-matrix cache fixes, then found and fixed several additional bugs in the expression-interpolation support (ExprInterpolationBlock) and the Constant/Function overload types.

Bugs fixed

1. Constant controls were silently downcast to Function during arithmetic

Function._ad_mul, _ad_add, _ad_copy, and _ad_create_checkpoint all hardcoded construction of a plain overloaded Function when building a new instance (and _ad_create_checkpoint relied on dolfinx.fem.Function.copy(), which itself always returns a plain Function regardless of subclass). Since Constant is a Function subclass, any of these operations run on a Constant — which pyadjoint calls internally during Taylor tests and optimization — silently produced a plain Function instead, breaking with TypeError: Control value must be an OverloadedType object with the same type as the control.

Fixed by adding Function._ad_new_like(), which bypasses the constructor (type(self).__new__(type(self), V) + Function.__init__(r, V)) to build a new instance of the correct concrete subclass sharing the same function space, since Constant.__init__ takes a different signature (mesh, value) than Function.__init__ (function_space) and can't be called generically.

2. import dolfinx_adjoint required scifem even if unused

blocks/interpolation.py had a module-level import scifem, and scifem is only needed by ExprInterpolationBlock for assembling Jacobian/Hessian matrices of UFL expressions. Since __init__.py eagerly imports interpolate, this meant the whole package failed to import for anyone without scifem installed, even if they only use the linear FunctionFunction interpolation path (or nothing interpolation-related at all). scifem also wasn't declared as a dependency anywhere.

Fixed by making the scifem import lazy (only at the two call sites that actually need it, raising a friendly pip install scifem error if missing), and adding scifem as an explicit optional-dependency group in pyproject.toml.

3. ExprInterpolationBlock re-derived dependency indices it already had

Every adjoint/TLM/Hessian method reimplemented a custom identity-matching lookup (get_dependency_index) to find a dependency's position, even though pyadjoint already passes the correct index directly (idx, and relevant_dependencies as (idx, block_variable) tuples) — confirmed by reading pyadjoint's own Block.evaluate_adj/evaluate_hessian. This was extra, fragile complexity for no benefit, and it also masked a shape bug in a manually-driven test (test_expr_interpolation_adjoint_property was calling prepare_evaluate_adj with the wrong relevant_dependencies shape — a bare list instead of (idx, block_variable) tuples).

Simplified all the block's evaluate/prepare methods to trust the index pyadjoint provides directly. Kept get_dependency_index itself as a small public helper, since it's still legitimately needed for tests that drive the block manually outside of pyadjoint's tape, and fixed that test's relevant_dependencies shape to match pyadjoint's real contract.

4. Two Taylor tests were missing a required re-evaluation before computing derivatives

test_expr_interpolation_taylor_test_function/_constant computed Jh.derivative()/Jh.hessian() without first re-calling Jh(control), so the tape was left checkpointed at the last perturbed point from the preceding first-order Taylor test rather than at the true control value — causing the second-order convergence check to fail at rate ≈1 instead of ≈3. Fixed by adding the missing Jh(control) call, matching the pattern already used in the analogous FunctionFunction interpolation test.

Verification

Full test suite passes (58/58), ruff check/ruff format --check clean, mypy clean (aside from one pre-existing, unrelated error in compat.py). Each fix was independently reproduced against the pre-fix commit to confirm the bug was real, and re-verified after the fix — including simulating a scifem-free environment and testing Constant arithmetic directly.

AI assistance

I used Claude Code (Claude Sonnet 5 and Claude Opus 5) to help implement, test, and iterate on this feature, and to draft this PR description. I reviewed, tested, and take full responsibility for the final contribution.

finsberg and others added 6 commits August 21, 2026 13:31
…rpolationBlock

The interpolation matrix cache was a module-level dict keyed by
(id(space_from), id(space_to)) without holding strong references, so a
garbage-collected FunctionSpace's id could be reused by an unrelated
object and silently return the wrong matrix. Cache the matrix as a
per-instance attribute on InterpolationBlock instead, tied to the
block's own space references.

recompute_component also wrapped a separately cached Function instead
of updating the tape's real output object, permanently disconnecting
it from the Python object returned by interpolate(). It now mutates
block_variable.saved_output in place, matching FunctionAssignBlock's
convention. Note this only holds until the output is used as a
dependency elsewhere, at which point pyadjoint freezes its own
checkpoint copy (an existing, codebase-wide characteristic, not
specific to interpolation) - documented inline.

Also fixes the mypy failures in this file: the missing
`dolfinx.fem.petsc` import that `petsc_mat=True` silently relied on
via import order, and the monkey-patched `_row_vec`/`_col_vec`
attributes on MatrixCSR.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…) reuse

The previous commit removed the shared matrix cache entirely (making it
per-instance on InterpolationBlock) to close the id()-collision hole,
but that gives up the redundant-assembly/MPI-communicator-exhaustion
protection the cache was originally for, since every new block (e.g.
each optimization iteration that creates fresh spaces) reassembles its
own matrix from scratch.

Bring back a shared, module-level cache, but keyed safely this time:
each FunctionSpace that contributes to a cache key gets a
weakref.finalize callback that purges every entry mentioning its id.
Finalizers run at the point the object is actually deallocated, which
is necessarily before CPython can hand that id out again, so a cache
hit always corresponds to spaces that are still alive - no stale
matrix can be served, and entries don't accumulate forever for
short-lived spaces.

Add regression tests: same-space-pair reuse, cache entries getting
purged once their space is garbage collected, and no unbounded growth
across many transient spaces. Verified these tests fail against a
naive id()-keyed cache without the weakref purge (i.e. they would have
caught the original bug).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
attach_working_array() dynamically attached _row_vec/_col_vec onto
dolfinx.la.MatrixCSR instances, which mypy can't see, so every access
needed typing.cast(Any, ...). Replace it with _MatrixCSRWorkspace, a
small class built once alongside the matrix that holds the working
vectors as real, statically-typed fields, threaded through get_mult(),
_build_interpolation_matrix(), and the cache in place of the bare
matrix. No behavior change (the full working arrays are still zeroed
before each multiply); no more casts or dynamic attributes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erpolate_expression

# Conflicts:
#	src/dolfinx_adjoint/blocks/interpolation.py
#	tests/test_interpolate.py
- Add the missing Jh_c(c) reset before derivative()/hessian() in the
  Constant Taylor test, mirroring the fix already applied to the
  Function variant (currently masked by the separate Constant
  downcast bug, but confirmed to fail on its own once that's fixed).
- Fix import order in the public interpolate() wrapper.
- Make ExprInterpolationBlock.evaluate_tlm_component re-derive the
  dependency index via get_dependency_index instead of assuming
  tlm_inputs is positionally aligned with self._deps, consistent with
  every other evaluate_*_component method in the class.
- Make ExprInterpolationBlock.recompute_component update
  block_variable.saved_output in place, matching
  InterpolationBlock.recompute_component's convention instead of a
  separately cached buffer wrapped in a fresh overloaded object.
- Function._ad_mul/_ad_add/_ad_copy/_ad_create_checkpoint all hardcoded
  construction of a plain overloaded Function, silently downcasting a
  Constant control during Taylor tests/optimization (surfaced as a
  Control-type TypeError). Add _ad_new_like(), which bypasses the
  constructor via __new__ + Function.__init__ to preserve the concrete
  subclass while still sharing the original function space.
- Make scifem import lazy in blocks/interpolation.py: only
  ExprInterpolationBlock's Jacobian/Hessian assembly needs it, so
  importing dolfinx_adjoint or using the linear InterpolationBlock no
  longer requires it to be installed. Add it as an optional dependency
  group in pyproject.toml (pulled in by the test extra).
- ExprInterpolationBlock's evaluate_*_component/prepare_evaluate_*
  methods no longer re-derive dependency indices via
  get_dependency_index: pyadjoint already hands back the correct index
  through idx and relevant_dependencies' (idx, block_variable) tuples,
  and self._deps is populated in lockstep with get_dependencies() in
  __init__, so position i means the same dependency in both. The
  helper itself is kept for external identity lookups (e.g. tests
  locating a specific control), and the test that manually drives the
  block is fixed to pass relevant_dependencies in pyadjoint's real
  (idx, block_variable) tuple shape instead of a bare block_variable
  list.
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.

1 participant