Generalise VectorField to N components - #5686
Conversation
Support 3D vector fields via N-component VectorField, Component/Norm operators, and matching discretisation/solution handling. Extracted from the unstructured finite-volume work for review in isolation. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5686 +/- ##
==========================================
+ Coverage 98.09% 98.11% +0.02%
==========================================
Files 340 340
Lines 32671 32732 +61
==========================================
+ Hits 32049 32116 +67
+ Misses 622 616 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add unit tests for the uncovered patch lines: Component/Norm discretisation (success and error paths), _unary_new_copy and the component/norm convenience functions, _disc_state_vector propagation through binary/unary operators, and the per-component casadi handling for VectorField variables in Solution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rtimms
left a comment
There was a problem hiding this comment.
Reviewed this PR, then checked the findings against the rest of the stack (ufv-1 … ufv-5) to avoid raising things that are already handled downstream. Verified behaviour by running the code at both this branch and the stack tip; targeted unit suites are green at both (1733 serialisation + expression-tree tests, 755 spatial-method/solver/discretisation tests).
Three things I think need fixing before this lands, then some dead surface and housekeeping.
Already covered downstream — ignore these
For the record, so they don't get re-raised:
MagnitudevsComponentduplication.Magnitude(vf, "lr"|"tb")is exactlyComponent(vf, 0|1), andMagnitudeis misnamed (it extracts a component; the newNormis the actual magnitude). #5691 handles this properly — deprecation warning, bothBasicDFN2Dcall sites migrated,FiniteVolume2D._edge_directionso edge-averaging still resolves a direction. Deprecating is the right call for a public symbol._disc_state_vectorhas a real writer. It's set atfinite_volume_unstructured.py:617in #5688, with genuine coverage attest_finite_volume_unstructured.py:874. See the note below about where it belongs, but the concept isn't dead.
Blockers
1. Mismatched component counts silently drop data
n is taken from whichever side happens to be a VectorField, with no check that both agree (discretisation.py:973-975). Once N can vary, that's a correctness hole:
2-comp + 3-comp -> returns 2 components; the third is silently dropped, no error
3-comp + 2-comp -> IndexError: list index out of range
Both reproduce on this branch. Suggest an explicit check before the loop:
if disc_left.n_components != disc_right.n_components:
raise pybamm.DiscretisationError(
f"Cannot combine VectorFields with {disc_left.n_components} and "
f"{disc_right.n_components} components"
)Worth fixing here rather than downstream, because finite_volume_unstructured.py:948-977 in #5688 is a near-verbatim second copy of this block — same n logic, same missing guard, same hasattr/break. Either fix both, or factor the "broadcast the scalar side, then zip components" step into one shared helper that both call.
2. solution["<vector field>"] is unreadable on structured 2D meshes
The solution.py change stores a list of casadi functions, but process_variable routes structured 2D FV meshes to ProcessedVariable2DFVM at processed_variable.py:1750, before the VectorField dispatch — and that dispatch is gated on UnstructuredSubMesh, so it never catches this case even at the tip of the stack. Result, verified on both this branch and ufv-5:
processed class: ProcessedVariable2DFVM
call(0.5) -> TypeError: unhashable type: 'list'
entries -> TypeError: unhashable type: 'list'
data -> TypeError: unhashable type: 'list'
Not a functional regression — on main this path raises TypeError: Cannot convert symbol of type VectorField to CasADi — but it swaps a clear message for an opaque one, on a path that stays broken through all six PRs. Either hoist an isinstance(base_variables[0], pybamm.VectorField) check above line 1750, or raise NotImplementedError there so the failure names the actual limitation.
Also worth noting: the added test only asserts isinstance(casadi_components, list), which is why it passes despite every read path raising. If the plumbing stays in this PR, the test should read a value.
Separately, if you'd rather drop the solution.py change from this PR and land it with its consumer in #5688, drop VectorField._to_casadi with it — with _to_casadi present and the special case gone, solution["flux"] silently returns wrong shapes instead of erroring.
3. hasattr sniffing of a private attribute across objects
result = pybamm.VectorField(*new_comps)
for src in (disc_left, disc_right):
if hasattr(src, "_disc_state_vector"):
result._disc_state_vector = src._disc_state_vector
break
return resultBy the tip of the stack this pattern exists in three places (discretisation.py:993, discretisation.py:1194, finite_volume_unstructured.py:974). Declaring the attribute removes the duck-typing from all three:
class VectorField(TensorField):
_disc_state_vector = Noneresult._disc_state_vector = disc_left._disc_state_vector or disc_right._disc_state_vector
# and in the unary branch
result._disc_state_vector = disc_child._disc_state_vectorGiven the only writer is in #5688, the propagation plus test_disc_state_vector_propagation would sit more naturally there — the test here has to fabricate the attribute by hand (disc_vf._disc_state_vector = marker), so it doesn't exercise anything this PR ships.
Dead surface
Grepped all six branches; none of the following has a single use in src/:
fb_field— unused even by the unstructured 3D models in #5690, andvf[2]already works viaTensorField.__getitem__. It's also documented as a "backward-compatible alias" when it's brand new. Suggest dropping it and its two tests until something needs a third component.pybamm.component()/pybamm.norm()— zero call sites anywhere in the stack; every real caller writespybamm.Component(N_e, 0). They're also bare constructor aliases with nosimplify_if_constant, unlike neighbours such assign().Norm— zero uses insrc/across the stack, andfinite_volume_unstructured.gradient_squared(#5688, lines 936-942) hand-rolls exactly the sum-of-squares that theNormdiscretisation branch builds. Either giveNormthat caller or defer it.
Reuse and style
TensorFieldalready exposescomponentsand__getitem__, but private_componentsaccess grows from 7 sites in this PR to 13 insrc/by the tip (processed_variable.py:1232,1238,finite_volume_unstructured.py:770,940,967). Worth switching to the public accessors while the count is still small.VectorField._to_casadireimplements the inherited helper;casadi.vertcat(*self._children_to_casadi(t, y, y_dot, inputs, casadi_symbols))is equivalent.- The
Normdiscretisation branch reads more directly asreturn sum(c**2 for c in disc_child.components) ** 0.5. n_componentsis a third spelling oflen(components)/shape[0]. Fine to keep, but the codebase should settle on one.- New disc branches raise bare
ValueError;AGENTS.mdasks forDiscretisationError. (The adjacentMagnitudebranch sets the precedent, but it'd be good not to extend it.)
Housekeeping
- The CHANGELOG bullet has no PR link —
AGENTS.mdrequires one, and the surrounding bug-fix bullets have them. Same applies to the other four feature bullets added across the stack. - New public
Component/Normhave nodocs/source/api/expression_tree/unary_operator.rstentry. (VectorField/TensorFieldwere already undocumented, so this is arguably pre-existing.)
|
Thanks for the thorough review — and for checking against the rest of the stack so we don't churn on things already handled downstream. Blockers
Dead surface
Reuse / style / housekeeping
Will push the follow-up shortly. |
Guard mismatched component counts, raise a clear error for structured-2D VectorField solution reads, and drop unused fb_field / convenience helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
`or` bool-evaluates StateVector and raises; use an explicit None check. Co-authored-by: Cursor <cursoragent@cursor.com>
Adds FiniteVolumeUnstructured (TPFA Laplacian, fused div_D_grad, Green-Gauss gradient), unstructured processed variables, and the discretisation dispatch for div(D*grad(u)) and graph-topology internal boundary conditions, on top of the unstructured meshing (#5687) and N-component VectorField (#5686) already on main. Includes the fixes from the review of the previous revision: - Neumann values on named axis sides are coordinate-direction derivatives (matching FiniteVolume); custom tags stay outward-normal - unknown BC sides and bc_types raise DiscretisationError instead of being silently dropped; boundary_integral supports "entire" - auxiliary domains are handled in laplacian/gradient BC assembly and in secondary/tertiary broadcasts - divergence of a BC-bearing flux raises rather than silently dropping the boundary flux; the Green-Gauss gradient warns for BC-less buckets - scalar reductions (Max/Min) on unstructured domains post-process as 0D - operator matrices are cached per submesh (invalidated when the cell ordering changes) and BC assembly is vectorised - ParameterSubstitutor.process_boundary_conditions processes every boundary side present, not a fixed whitelist, so tab and named-region Dirichlet conditions are no longer silently dropped Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
VectorFieldfrom fixed 2D(lr, tb)to N components (needed for 3D unstructured FV).Component/Normoperators and matching discretisation + solution handling.Stack
Full pre-split branch preserved as
backup/unstructured-finite-volume-fulland original #5397 (unstructured-finite-volume).Test plan
test_finite_volume_2d/test_tensor_field.py(includes new VectorField N-comp cases)Also in this stack: #5691 deprecates
pybamm.Magnitude.