Finsberg/interpolate expression - #65
Open
finsberg wants to merge 6 commits into
Open
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merged in
finsberg/interpolate_func_into_spaceto pick up its interpolation-matrix cache fixes, then found and fixed several additional bugs in the expression-interpolation support (ExprInterpolationBlock) and theConstant/Functionoverload types.Bugs fixed
1.
Constantcontrols were silently downcast toFunctionduring arithmeticFunction._ad_mul,_ad_add,_ad_copy, and_ad_create_checkpointall hardcoded construction of a plain overloadedFunctionwhen building a new instance (and_ad_create_checkpointrelied ondolfinx.fem.Function.copy(), which itself always returns a plainFunctionregardless of subclass). SinceConstantis aFunctionsubclass, any of these operations run on aConstant— which pyadjoint calls internally during Taylor tests and optimization — silently produced a plainFunctioninstead, breaking withTypeError: 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, sinceConstant.__init__takes a different signature (mesh, value) thanFunction.__init__(function_space) and can't be called generically.2.
import dolfinx_adjointrequiredscifemeven if unusedblocks/interpolation.pyhad a module-levelimport scifem, andscifemis only needed byExprInterpolationBlockfor assembling Jacobian/Hessian matrices of UFL expressions. Since__init__.pyeagerly importsinterpolate, this meant the whole package failed to import for anyone withoutscifeminstalled, even if they only use the linearFunction→Functioninterpolation path (or nothing interpolation-related at all).scifemalso wasn't declared as a dependency anywhere.Fixed by making the
scifemimport lazy (only at the two call sites that actually need it, raising a friendlypip install scifemerror if missing), and addingscifemas an explicit optional-dependency group inpyproject.toml.3.
ExprInterpolationBlockre-derived dependency indices it already hadEvery 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, andrelevant_dependenciesas(idx, block_variable)tuples) — confirmed by reading pyadjoint's ownBlock.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_propertywas callingprepare_evaluate_adjwith the wrongrelevant_dependenciesshape — 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_indexitself 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'srelevant_dependenciesshape 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/_constantcomputedJh.derivative()/Jh.hessian()without first re-callingJh(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 missingJh(control)call, matching the pattern already used in the analogousFunction→Functioninterpolation test.Verification
Full test suite passes (58/58),
ruff check/ruff format --checkclean,mypyclean (aside from one pre-existing, unrelated error incompat.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 ascifem-free environment and testingConstantarithmetic 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.