Repository navigation
New fix - #1
Merged
NicolaCourtier merged 12 commits intoSep 17, 2026
Merged
New fix#1NicolaCourtier merged 12 commits into
NicolaCourtier merged 12 commits into
Conversation
* Re-implement speed benchmarks in pytest-benchmark * Remove casadi from benchmarks * Add .benchmarks to gitignore * Add memory benchmarks * Just mark speed benchmarks Full suite is fast enough without casadi to not need a separate 'slow-bench' split. * Delete ASV benchmarks * Add workflow to run benchmarks against main * add continue on error temporarily to benchmark pr * Increase the memory limit for test_parameterise_memory * Bump simulation_setup_memory limit * Stop pytest auto-assignment under benchmarks * tests: stop pytest collecting benchmarks unless path explicitly passed * ci: delete old asv benchmark workflows * benchmarks: switch ScipySolver to IDAKLU in unit benchmarks * benchmarks: rename speed_bench to time_bench * benchmarks: update readme * ci: start to build in bencher to benchmarks CI * ci: switch to bencher bare-metal * ci: reduce number of tests run on Bencher to stay under 5min limit * Add to changelog * benchmarks: fix codacy error --------- Co-authored-by: Martin Robinson <martinjrobins@gmail.com>
* feat: add unstructured finite volume spatial method 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 (pybamm-team#5687) and N-component VectorField (pybamm-team#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> * fix: tile per-face BC vectors across auxiliary-domain repeats in div_D_grad A BC value with one entry per boundary face broke with a ShapeError when the variable had auxiliary domains; lift it to n_bnd * repeats entries, matching _bc_contribution's convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: raise on y= queries of 2D unstructured processed variables y was silently dropped when building the query grid, returning z-midplane values for a query the user thinks is at y; raise like the existing r/R check. 3D unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf: interpolate all time steps of unstructured variables in one pass scipy's interpolators accept (n_points, n_t) value arrays, so the nearest-neighbour KDTree and the domain boundary mask are now built once per query instead of once per time step (~40x on a 100-step animation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: raise DomainError for unresolvable unstructured spatial variables spatial_variable silently fell back to the x column for unknown names and directions, matched loose prefixes (zeta -> z), and accepted y/fb on 2D x-z meshes. Match the leading name token exactly and raise otherwise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor: define set_internal_bcs_for_concat on the base SpatialMethod Replace the hasattr duck-typing dispatch with a documented base-class hook returning None (use the default 1D-stack routine); the unstructured method's implementation becomes a plain override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: match legacy tab BC sides exactly instead of by substring With arbitrary boundary tags now allowed, a region name containing 'tab' (e.g. 'tab_weld') was misrouted into check_tab_conditions and raised ModelError; share one exact-name set across both check sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style: pre-commit fixes * fix: fail loudly when unstructured meshes lack tags or interface data build() now warns for a submesh with exterior faces but no boundary tags (BCs cannot apply, interface discovery cannot pair it), and _internal_neumann_unstructured raises instead of returning zeros when no interface pairs two meshes, which silently decoupled the domains. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add implicit non-orthogonal correction to the unstructured TPFA operators The two-point flux only carried alpha (u_j - u_i)/d with alpha = cos(theta) and dropped the k . grad(u) remainder of n = alpha e + k, so the Laplacian of a linear field was nonzero on any non-orthogonal mesh and the scheme did not converge on triangles or tetrahedra (L2 rate ~0 under refinement). - Split the face normal as n = alpha e_ij + k and add the k . grad(u)_f cross flux implicitly, for internal faces (laplacian, div_D_grad) and Dirichlet boundary faces (perpendicular distance delta . n). - "non-orthogonal correction" option: "over-relaxed" (default, alpha = 1/cos theta, floored) or "minimum" (alpha = cos theta). - Replace the Green-Gauss gradient with a batched weighted least-squares reconstruction that is exact on linear fields on skewed meshes; internal, Dirichlet and Neumann faces each contribute one directional-derivative row. Green-Gauss remains the divergence assembly. - Raise GeometryError for inverted cells, warn at build above 70 degrees of non-orthogonality, skip the correction entirely on orthogonal meshes. - _divergence_matrices now aliases _green_gauss_matrices. Linear patch test is at 1e-13 on tri, perturbed tri and Kuhn tet meshes; Poisson convergence rates are 1.95-2.04 (tri) and 1.74-1.88 (tet) for both options, unchanged (2.0) on quad/hex. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Test the build-time non-orthogonality warning; make zip strict Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Return a pybamm.Matrix from the unstructured definite integral definite_integral_matrix returned a raw csr_matrix, so DefiniteIntegralVector failed with AttributeError in process_symbol's shape check; it also ignored vector_type (a "column" request came back as a row) and integral() dropped integration_dimension, silently integrating the primary volume for a secondary-dimension Integral. Support row/column, raise NotImplementedError for non-primary dimensions, and lift by the auxiliary-domain repeats. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Move point-in-domain onto UnstructuredSubMesh; skip nearest fill outside it UnstructuredSubMesh.contains_points now owns the 2D even-odd loop test (loops cached on the mesh) and delegates to contains_points_3d, so the processed variable only asks the mesh. In _interpolate_spatial the outside mask is computed first and excluded from the nearest-neighbour fill, which previously filled ~97% of out-of-hull points only to overwrite them with fill_value. Also correct the get_quiver_data docstring for the 3D return. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Apply the non-orthogonal correction across unstructured domain interfaces The interface Neumann value between concatenated domains was the plain two-point difference (u_r - u_l)/d, the same alpha = 1 truncation that made the Laplacian inconsistent inside a domain, so a linear field was not a steady state of a two-domain tet model (residual 4.6 on the Kuhn mesh). - internal_neumann_condition now returns alpha (u_r - u_l)/d + k . grad(u)_f with the same n = alpha e + k split as the interior; the face gradient is the distance-weighted mean of both sides' least-squares gradients, which take the interface faces as cross-mesh rows towards the paired cell and each side's external boundary conditions (passed by set_internal_bcs_for_concat). Orthogonal interfaces are unchanged. - interface_data records left_faces/right_faces on both pairing paths. - Least-squares rows for boundary faces without a condition now fit a zero normal derivative, matching the operators' zero-flux treatment, so cells on untagged boundaries stay fully determined. - UnstructuredSubMesh.contains_points gets a direct test. Two-domain linear field on tets: residual 4.6 -> 5e-14; interface flux is conservative to 1e-10 for a random field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Test the corrected interface gradient on a linear field The old expectation hard-coded the two-point (u_r - u_l)/d formula, which is only the full interface gradient on orthogonal pairs; check u = x gives exactly 1 on the skewed triangle interface and keep the two-point check for a quad interface where it still holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Rename the axis token variable that Bandit mistakes for a password Codacy's Bandit B105 flags `token == "y"` as a hardcoded password purely because of the variable name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: drop duplicate Component/Norm entries after merging main main already documents both in unary_operator.rst; the second copy made Sphinx fail with "duplicate object description" under -W. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cover the inconsistent-face-count error and the no-loops containment path Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Treat every scalar-shaped boundary value as a broadcast scalar Time- or input-dependent scalars (e.g. a current-density Neumann value) evaluate for shape to () rather than (1, 1), so _bc_contribution built Matrix @ scalar and failed once div_D_grad started assembling the least-squares gradient with boundary rows. Centralise the check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use the harmonic mean of D at unstructured faces in div(D * grad(u)) The face coefficient was a linear interpolation of the two cell values. For a flux the physically consistent face value is the resistances-in- series (distance-weighted harmonic) mean, which FiniteVolume already uses for coefficients of a gradient: it reproduces the exact two-cell flux across a material interface and gives zero flux into an impermeable cell, whereas the arithmetic mean lets half the conductive side leak through. On combined concatenation meshes this is the current-collector/electrode face, where sigma jumps by ~1e5. Linear weights are kept for the face gradient of the cross term; D must be strictly positive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Treat faces orthogonal to within 1e-8 as orthogonal, not 1e-12 Centroid rounding on high-aspect-ratio cells (10 um thick, cm wide, as in a pouch cell) puts ~1e-11 into the face direction, so with a 1e-12 cut-off the cross term and boundary cross terms were assembled on every hex mesh with coefficients of 1e-11. That doubled the Jacobian stencil (57k -> 101k nonzeros on the pouch demo) and the IDAKLU time for no change in the answer. k is now zeroed below a dimensionless 1e-8 (an angle far below any real skew) and explicit zeros are dropped, so orthogonal faces never widen the stencil, also on mixed meshes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Keep plotting out of the unstructured processed variables The visualisation grid (N_VIS/N_VIS_3D and the first/second/third_dim_pts it populated), mid-plane 3D slices, quiver sampling and slice positions only exist to feed QuickPlot, whose unstructured support lives in the plotting PR. Processed variables here now only interpolate at requested points; QuickPlot refuses unstructured variables with a clear message until that support lands, instead of failing deep inside its 2D branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* bug: remove job_name from benchmark_memory * ci: update bencher key * Revert "ci: update bencher key" This reverts commit 06bc2e1. * empty commit to force benchmark re-run
* Fix parameter discovery in Variable metadata (pybamm-team#5752) * Update changelog link for pybamm-team#5753 * Move variable metadata unpacking to SymbolUnpacker
…updates group across 1 directory (pybamm-team#5742) Signed-off-by: dependabot[bot] <support@github.com>
…pybamm-team#5748) Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> Signed-off-by: dependabot[bot] <support@github.com>
…th 13 updates (pybamm-team#5749) Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> Signed-off-by: dependabot[bot] <support@github.com>
….0 (pybamm-team#5741) Signed-off-by: dependabot[bot] <support@github.com>
NicolaCourtier
merged commit Sep 17, 2026
398443e
into
NicolaCourtier:step-cycle-numbers
36 checks passed
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
6 participants
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.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests