Skip to content

Scalar/VectorField renaming: unguarded name collisions and inconsistent naming contracts #1299

Description

@PhilipDeegan

Mechanism.

VectorField.init (vectorfield.py:17) and ScalarField.init (scalarfield.py:7) call compute_rename, it zips new_names=("x","y","z") against whatever is in the patch_datas dict in iteration order, ignoring the original names (hierarchy_utils.py:459-474). Once wrapped, every VectorField's quantities become literally "x", "y", "z" — no trace remains of what they originally were (flux, bulk velocity, current, MHD momentum, ...). When compute_hier_from merges multiple hierarchies, extract_patchdatas flattens all of their patch_datas dicts into one dict keyed only by name (hierarchy_utils.py:172-180), so two hierarchies sharing a name silently overwrite one another. No guard exists against this: are_compatible_hierarchies (hierarchy_utils.py:82-93) computes same_box/same_selection/same_files/same_times and then unconditionally returns True, discarding all of them.

Consequence. The generic renaming scheme is safe today only because, in every call site that currently exists, no two hierarchies being merged happen to carry the same generic name at the same time. That is an accident of the current call graph, not a property enforced anywhere in the code.

Where it shows up in run.py:

  • GetPressure (run.py:137-149) merges M (raw prefixed names like popname_Mxx), V (a VectorField renamed to x/y/z), and N (a ScalarField renamed to value) via a plain compute_hier_from call. It works only because none of the three share a name yet. Extending it to combine two vector quantities at once (e.g. two populations' fluxes, or Vi alongside a population flux) would hit the collision directly, with no error raised — the exact case originally raised. Contrast this with ScalarField.add/VectorField.add (scalarfield.py:17-32, vectorfield.py:39-60), which already handle this by renaming operands to self_x/other_x before merging; GetPressure uses neither that convention nor any other.
  • GetPi and GetPressure compute the same kind of physical quantity (pressure from a momentum tensor plus a velocity-like vector) through two different naming contracts. GetPi (run.py:151-156) passes the raw bulk-velocity hierarchy into _compute_pressure, which looks up literal "Vx"/"Vy"/"Vz" (utils.py:444-446). GetPressure passes a VectorField-wrapped flux into _compute_pop_pressure, which looks up the generic "x"/"y"/"z" (utils.py:482-484). Swapping which form is passed into either function produces a bare KeyError, since neither function documents which naming contract it expects.
  • GetB, GetE, GetMHDV, GetMHDrhoV, GetJ (run.py:104,114,203,226,176) rename to x/y/z twice: once explicitly via _compute_to_primal's kwargs, then again positionally inside VectorField.init. The second rename is currently a no-op, contingent on dict order being preserved between the two steps.
  • GetVi/GetFlux (run.py:129-135) skip the explicit _compute_to_primal rename and rely on VectorField.init's positional rename directly off the raw HDF5 field order — a different route to the same end state than GetB/GetE take, with no stated reason for the difference.
  • GetMHDrho/GetMHDP/GetMHDEtot use value="mhdX" as the _compute_to_primal kwarg (run.py:193,213,239), while GetPe uses scalar="rho" (run.py:166). This has no functional effect, since compute_rename ignores the kwarg name entirely, but it shows the kwarg name carries no real meaning despite appearing to.

Overall picture: three unstated assumptions currently hold the generic-renaming scheme together — that dict iteration order matches component order, that no two hierarchies merged in the same compute_hier_from call ever produce the same generic name, and that are_compatible_hierarchies would catch it if they did. The third assumption is false as written (dead code), leaving the second as the only real protection, and that protection exists by coincidence of the current call sites rather than by design.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    Status
    thinking... 🤯

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions