Tracking: internal inconsistencies in naming, APIs and value handling

# Tracking: internal inconsistencies in naming, APIs and value handling

A survey of inconsistencies across `src/festim/`, grouped by kind rather than by file
since most of them repeat across many modules. Nothing here is a bug report; it is a
list of places where the codebase says the same thing in two or more ways, which makes
new code harder to write consistently and forces callers to special-case.

Line numbers are as of `71e8781b` (2026-07-24). Items are grouped so that each one could
become its own PR; the checkboxes are meant as a tracking list.

## 1. Two vocabularies for the same topology

The mesh entity words are mixed at every level. There is already a
`# TODO: fix naming inconsistency between facet and surface meshtags` at
`src/festim/problem.py:119`.

- `ProblemBase` stores `facet_meshtags` and `volume_meshtags` (`problem.py:60-61`).
That pairs an entity word (facet) with a domain word (volume); the consistent pairs
would be facet/cell or surface/volume.
- `MeshFromXDMF` uses the other pairing for the same two things:
`surface_meshtags_name` / `volume_meshtags_name` (`mesh/mesh_from_xdmf.py:36-43`),
with builders `define_surface_meshtags()` / `define_volume_meshtags()`
(`mesh_from_xdmf.py:50`, `:63`).
- `Mesh.define_meshtags(surface_subdomains=..., volume_subdomains=...)` takes
surface/volume in and returns `(facet_meshtags, volume_meshtags)` out
(`mesh/mesh.py:130`, `:258`). The words swap inside one function.
- `Mesh` exposes `vdim`/`fdim` (`mesh/mesh.py:113-125`), a third pairing (volume/facet)
for cell dim and facet dim.
- Abbreviated attributes have no shared convention and one is misleading:
`VolumeSubdomain.ft` is facet tags (`subdomain/volume_subdomain.py:158`), while
`Interface.mt` holds the *volume* meshtags (`hydrogen_transport_problem.py:1355`).

Public API in two places (`MeshFromXDMF` kwargs, `problem.facet_meshtags`), so this
needs the alias + `DeprecationWarning` route.

- [ ] Pick one pairing and migrate

## 2. The region a term attaches to has three argument names

- `volume=` on `ParticleSource` (`source.py:73`), `Reaction` (`reaction.py:66`),
`InitialConcentration` (`initial_condition.py:113`), `TotalVolume`.
- `subdomain=` on `AdvectionTerm` (`advection.py:37`), all BCs (where it is a
*surface*), and `CustomQuantity` (where it is either).
- `surface=` on the `SurfaceQuantity` family.

So `subdomain` means "surface" in `ParticleFluxBC`, "volume" in `AdvectionTerm` and
"either" in `CustomQuantity`, while the class names `SurfaceSubdomain` /
`VolumeSubdomain` say that both *are* subdomains.

- [ ] Decide on a rule (e.g. `subdomain=` everywhere, or always the specific word)

## 3. `species` vs `field`

The exports call it `field` (`exports/surface_quantity.py:33`, `VTXSpeciesExport.field`),
everything else calls it `species` (BCs, sources, advection, reactions).

Both accept a `str` name to be resolved later, but only three call sites do the
resolving (`hydrogen_transport_problem.py:507-511`, `:766`), so whether passing a string
works depends on the class.

- [ ] Rename `field` to `species`, keeping `field` as a deprecated alias
- [ ] Resolve string names in one place, or drop string support

## 4. The value-conversion family (largest duplication)

Seven near-copies of "turn a user value into a fenics object", with four method names and
three different parameter sets:

| Where | Method | Takes |
|---|---|---|
| `helpers.py:285` | `Value.convert_input_value` | `function_space, t, temperature, up_to_ufl_expr, subdomain` |
| `boundary_conditions/flux_bc.py:90` | `FluxBCBase.create_value_fenics` | `mesh, temperature, t` |
| `boundary_conditions/flux_bc.py:203` | `ParticleFluxBC.create_value_fenics` | same, re-pasted with a species tweak |
| `boundary_conditions/dirichlet_bc.py:357` | `FixedConcentrationBC.create_value` | `function_space, temperature, t, K_S` |
| `boundary_conditions/dirichlet_bc.py:475` | `FixedTemperatureBC.create_value` | `function_space, t` |
| `species.py:192` | `ImplicitSpecies.create_value_fenics` | `mesh, t` |
| `boundary_conditions/surface_reaction.py:57` | `SurfaceReactionBCpartial.create_value_fenics` | `mesh, temperature, t` |
| `enclosure/enclosure.py:166` | `convert_input_values_to_fenics_objects` | `function_space, t` |

All contain the same `if isinstance(value, int|float) ... elif callable ... "t" in args
and "x" not in args` ladder.

Downstream names differ too:

- result attribute: `value_fenics` (BCs, `ImplicitSpecies`), `fenics_object` (`Value`),
`expr_fenics` (`InitialConcentration`, `initial_condition.py:110`)
- cached interpolation expression: `bc_expr` (BCs) vs `fenics_interpolation_expression`
(`Value`)
- predicates: `time_dependent` (`flux_bc.py:66`) vs `explicit_time_dependent`
(`helpers.py:251`), plus a per-class copy of `temperature_dependent` in three files
- updater: `update()` (BCs, `Value`) vs `update_density()`
(`ImplicitSpecies`, `species.py:231`)

Mesh vs function space is a substantive split, not just naming: half these methods take
a `mesh` and therefore cannot build a `fem.Function`, the other half take a
`function_space` and can.

Already targeted by the `Value` rework (#1182).

- [ ] Route every value through `helpers.Value`
- [ ] Settle on one name for the converted object and one for the cached expression

## 5. `.value` means two different things to a user

```python
F.ParticleSource(value=2.0, ...).value # -> festim.helpers.Value object
F.ParticleFluxBC(value=2.0, ...).value # -> 2.0
```

`SourceBase.value`'s setter wraps into `Value` (`source.py:52-59`), as do the enclosure
and opening classes, while BCs and initial conditions keep the raw input and expose the
converted form separately. Reading back what you passed in gives a different type
depending on the class.

This is the item on the list that is user-facing rather than cosmetic.

- [ ] Make `.value` mean the same thing everywhere (raw input, with the wrapper private)

## 6. `compute()` has no contract, so callers type-switch

`DerivedQuantity.compute` is declared `(*args, **kwargs)`
(`exports/derived_quantity.py:29-30`) and the concrete signatures diverge completely:

- `SurfaceFlux`, `TotalSurface`, `AverageSurface`: `compute(u, ds, entity_maps)`
- `TotalVolume`, `AverageVolume`: `compute(u, dx, entity_maps)`
- `Minimum/MaximumSurface`, `Minimum/MaximumVolume`: `compute()` with no arguments,
reading a `facet_meshtags` attribute the problem injects from outside
(`hydrogen_transport_problem.py:543-545`, `exports/minimum_surface.py:32`)
- `CustomQuantity`: `compute(measure, entity_maps)`
- `GasPressure`: `compute()`

So `post_processing` must isinstance-dispatch on the concrete subclass to know how to
call it (`hydrogen_transport_problem.py:1100-1140`), and that block is duplicated in the
discontinuous problem (`:2777-2820`) with different conventions: positional
`export.compute(export.field.solution, self.ds)` in one, keyword
`u=..., ds=..., entity_maps=...` in the other. Adding an export subclass currently means
editing the problem classes.

Two smaller things inside that family:

- `AverageVolume.compute` calls `assemble_scalar(u * dx(...))` on a raw UFL form while
its surface twin wraps in `fem.form(...)` (`exports/average_volume.py:23-30` vs
`exports/average_surface.py:37-39`)
- the same method drops `entity_maps` from its normalisation term while keeping it in
the numerator

- [ ] Give `compute()` a single signature and remove the isinstance dispatch

## 7. Five verbs for "build this during setup"

Within one class: `define_temperature`, `define_function_spaces`, `define_D_global`,
`define_boundary_conditions`, `create_formulation`, `create_initial_conditions`,
`create_flux_values_fenics`, `create_implicit_species_value_fenics`,
`convert_source_input_values_to_fenics_objects`,
`convert_advection_term_to_fenics_objects`, `initialise_exports`,
`assign_functions_to_species`.

The same job also gets different names across sibling classes:

- `HeatTransferProblem.create_source_values_fenics` (`heat_transfer_problem.py:132`) vs
`HydrogenTransportProblem.convert_source_input_values_to_fenics_objects`
(`hydrogen_transport_problem.py:816`)
- `define_function_space` (singular, heat) vs `define_function_spaces` (plural, hydrogen)

- [ ] Agree on one verb for setup methods and rename on touch

## 8. The surface-to-volume map has two incompatible APIs

- `HydrogenTransportProblem` exposes a **method**, `volume_subdomain_of_surface(surface)`,
with a lazily built private dict and an explanatory error
(`hydrogen_transport_problem.py:602-637`)
- `HydrogenTransportProblemDiscontinuous` exposes a public **dict attribute**,
`surface_to_volume[...]` (`:1231`, `:1313`)

Both wrap the same `map_surface_to_volume_subdomains`. The dict version is why lookups
that miss (e.g. a codim-2 surface subdomain, which is deliberately absent from the map)
surface as a bare `KeyError` instead of the method's `ValueError`.

- [ ] Keep one of the two

## 9. Class and module names

- `SurfaceReactionBCpartial` (`boundary_conditions/surface_reaction.py:9`) is the only
class not in CapWords
- base classes are `SourceBase`, `FluxBCBase`, `DirichletBCBase`,
`InitialConditionBase`, `OpeningBase`, `ProblemBase`, `InterfaceBase`, and then
`ExportBaseClass` (`exports/vtx.py:16`)
- `convergenceTest` is camelCase and publicly exported (`__init__.py:68`)
- "module filename = snake_case of the main class" holds across `exports/` except
`vtx.py`, which holds `ExportBaseClass`, `VTXTemperatureExport`, `VTXSpeciesExport`,
`CustomFieldExport` and `ReactionRateExport`
- export class names split between suffixed (`XDMFExport`, `Profile1DExport`,
`VTXSpeciesExport`) and unsuffixed (`SurfaceFlux`, `TotalVolume`, `GasPressure`)
- `find_surface_from_id` and `find_volume_from_id` are exported publicly but never called
anywhere in `src/` (only `find_species_from_name` is used)

- [ ] Rename `SurfaceReactionBCpartial`, `ExportBaseClass`, `convergenceTest`
- [ ] Split `exports/vtx.py`
- [ ] Decide whether the unused `find_*_from_id` helpers stay public

## 10. Validation: three mechanisms, and a type that cannot work

- setters that raise at assignment (the target rule): `Mesh`, `SourceBase`, `Reaction`,
`SurfaceSubdomain.dim`, others
- `assert` for the same job elsewhere, notably `subdomain/volume_subdomain.py:136`,
`:147`, and in `dirichlet_bc.py`. Asserts vanish under `python -O` and carry no message
- some checks only fire during `initialise()`

`SurfaceQuantity.surface` accepts `int | SurfaceSubdomain`
(`exports/surface_quantity.py:56-59`) and `field` accepts `Species | str`, but nothing
ever converts an int surface into a subdomain and `title` immediately does
`self.surface.id`. The int branch cannot work; it only moves the failure somewhere worse.

Message templates vary: `"volume must be of type festim.VolumeSubdomain"` (no received
type), `"surface should be an int or F.SurfaceSubdomain"` (should / `F.` prefix),
`"Name must be a string"` (capitalised attribute), `"dim must be an integer or None, not
{type(value)}"` (the target template). Roughly half omit the received type.

- [ ] Replace validation asserts with setter checks
- [ ] Drop the unusable `int` branch on `SurfaceQuantity.surface`
- [ ] Sweep error messages onto the standard template

## 11. Typing and docstrings

- `Union[...]` / `Optional[...]` / `List[...]` survive in `species.py:148`,
`initial_condition.py:110`, `reaction.py:16-29`, `exports/vtx.py:223`, sometimes mixed
with the modern form in one annotation: `Union[_Species, list[_Species]] | None`
(`reaction.py:68`). `species.py:148` annotates `Union[float, callable]`, using the
builtin function `callable` as a type
- 27 of 54 modules still repeat types in docstrings
(`value (float, fem.Constant, callable): ...`)
- the `:: testcode::` typo that silently skips a doctest is live in two files,
`reaction.py:39-43` and `species.py:47-51`, so those two classes have no executed
examples
- `reaction_term` uses `Arguments:` instead of `Args:` (`reaction.py:120`), which
napoleon does not render as a parameter list
- `isinstance` is written both as a tuple (10 sites) and as a union (33 sites)
- one mutable default survives: `product: ... = []` (`reaction.py:68`)

- [ ] Fix the two `:: testcode::` typos (quick win, restores two doctests)
- [ ] Modernise the remaining `typing` imports and drop docstring types on touch

## 12. Singular names holding lists, half-done normalisation

`Reaction.reactant` is singular but its setter always normalises to a list
(`reaction.py:83-97`). `Reaction.product` is singular, has no setter, and stays whatever
the user passed, so every consumer re-normalises it
(`products = self.product if isinstance(self.product, list) else [self.product]` in
`__repr__`, `__str__`, `reaction_term`, and again in the problem classes). Same pattern
with `Species.subdomains`, typed `list[VolumeSubdomain] | VolumeSubdomain | None`.

`Species` also offers two access styles for one thing: the `concentration` property and
the `concentration_submesh(subdomain)` method, plus a `legacy` property that decides
which world you are in by checking whether a dict is empty (`species.py:120-127`).
`ImplicitSpecies` does not subclass `Species`, names its magnitude `n` rather than
`value`, and duplicates both accessors.

- [ ] Normalise `product` in a setter like `reactant`
- [ ] Decide the future of `Species.legacy` and the two concentration accessors

## 13. Tests

- no `conftest.py` anywhere, and only one file uses fixtures at all (`test/test_mesh.py`);
shared dummies are rebuilt per file
- two shared-helper modules with different names and no clear split: `test/utils.py` and
`test/system_tests/tools.py`
- file naming splits: `test_dirichlet_bc.py`, `test_flux_bc.py` vs `test_henrysbc.py`,
`test_sievertsbc.py`
- `test/system_tests/tools.py` builds three module-level meshes at import time, including
a 20³ cube, so every system test pays for them (`tools.py:10-12`); it also imports MPI
as `import mpi4py.MPI as MPI`, against the isort mpi-section convention used elsewhere

- [ ] Introduce `conftest.py` fixtures for the common dummies
- [ ] Merge or clearly split `utils.py` and `tools.py`

## Suggested priority

1. **#5** and **#6** change behaviour rather than appearance: `.value` returning
different types, and exports being unextendable without editing the problem classes.
2. **#4** is the largest volume of duplicated code and is already in flight via #1182.
3. **#11**'s doctest typo is a two-line fix that restores CI coverage of two public
classes.
4. **#1** and **#2** are the widest-reaching but purely mechanical, and both touch public
API, so they want their own deprecation PRs rather than boy-scouting.

Start by choosing one checkbox rather than treating this tracking issue as a single change, then read the cited entry points such as src/festim/problem.py, src/festim/mesh/mesh.py, and src/festim/helpers.py. The selected item is done only when its affected APIs are made consistent, aliases or warnings are handled where noted, and the relevant existing behavior remains covered; this issue names no specific test to run.

Written by the indexing model from the issue text.