[joshmann-inversionsemi]

Adds an _ExternalField class which covers adding external fields in FBPIC: fbpic.lpa_utils.external_fields.ExternalField

• Add serializable UniformExternalField and ExpressionExternalField configurations, the former being a uniform and constant field profile and the latter using the MathExpression functionality for general field functions
• Support multiple field components, named particle-species targeting, boosted-frame transforms, and optional field override behavior
• Track runtime particle-name-to-Particles mappings in Simulation; refactor shared particle-diagnostic registration for plasma and beam species
• Add external-field usage to the PWFA demo
• Harden expression callback compilation with reusable namespaces and reserved FBPIC parameter validation

[joshmann-inversionsemi]

@claude , please review this PR.

[claude]

Claude finished @joshmann-inversionsemi's task in 2m 27s —— View job

---

PR Review: FBPIC external fields

• Gather context (diff, related files)
• Review external_field.py / .pyi
• Review math_expression.py changes
• Review simulation.py refactor
• Review tests & demo
• Post review

This PR is well structured. The _ExternalField / UniformExternalField / ExpressionExternalField split matches the existing _DensityProfile and _ParticleBeam patterns. Serialization round-trips, external fields are part of the config hash, and _add_particle_diagnostics removes a large amount of duplicated code. I found one real correctness bug, one physics question to settle before merge, and a few smaller items.

---

🔴 Bug: a parameter named after a callback argument is silently replaced

Where: external_field.py:111-131 and external_field.py:184-193

The generated callback is:

def field_function(gathered_field, x, y, z, t, amplitude, length_scale):
    return gathered_field + amplitude * (<expr>)

The expression's parameters are passed in as module globals through compilation_namespace(). Inside the function, any argument with the same name is a local and takes precedence. _expression_field_mapping blocks amplitude and length_scale, but it does not block the other argument names. Two cases still get through:

• A coordinate the expression doesn't declare as a variable. For example:

  MathExpression(expression="B0 * np.exp(-z / t)", variables=("z",), parameters={"B0": 1.0, "t": 1e-3})

  This passes validation. In the callback, t is the simulation time, not 1e-3, so the result is silently wrong.
• gathered_field. A parameter with this name would evaluate as the grid field.

Nothing errors in either case; the physics is just wrong.

Suggested fix: reject any parameter named after a callback argument:

_RESERVED_CALLBACK_PARAMETERS = frozenset(
    {"gathered_field", "x", "y", "z", "t", "amplitude", "length_scale"}
)

Also add a test using parameters={"t": ...} with variables=("z",). A sturdier option is to generate the argument names (e.g. _F, _x, _y, _z, _t, _amp, _ls) and bind the coordinates by name explicitly, so user names can never collide.

Fix this →

---

🟠 Please confirm: transverse fields in boosted-frame runs

Where: the _ExternalField docstring, external_field.py:138-140

The docstring says FBPIC "transforms the fields and coordinates to the boosted frame". Each FBPICExternalField is built for a single component, though. Under a boost, transverse components mix (e.g. Ex' = γ(Ex − βc·By), By' = γ(By − βEx/c)). A lab-frame By therefore also has to contribute to Ex'.

I couldn't open the FBPIC source in this environment, so I haven't confirmed whether ExternalField(gamma_boost=...) transforms only the coordinates or also the field values. Please check this before relying on it:

• If FBPIC transforms only the coordinates, Ex/Ey/Bx/By will be wrong in boosted runs. They would need to be cross-coupled here, or rejected when gamma_boost is set.
• Ez/Bz are invariant under a boost along z, so the demo's 1 T Bz is unaffected either way.

---

🟡 Smaller issues

1. Only part of the expression language works in callbacks (math_expression.py:373-389, ExpressionExternalField). FBPIC compiles field_func with Numba, but MathExpression accepts things Numba likely won't compile here. Examples: sp.special.* (not supported without numba-scipy), list/tuple literals with np.linalg.norm([...]), and the complex constant i. These failures would surface deep inside FBPIC setup rather than at config time. At minimum, document the supported subset in the ExpressionExternalField docstring. Consider rejecting sp/scipy and i in _expression_field_mapping.

2. The units are documented as "dimensionless" (UniformExternalField docstring, external_field.py:207). The value goes straight to FBPIC with amplitude=1.0, so it is in SI units: V/m for E* and T for B*. The demo's README already reads it as "1 T". Please say so in the docstring and the .pyi.

3. Non-finite uniform strengths fail late. _uniform_field_mapping accepts inf and nan, and repr(float("inf")) gives inf. The uniform namespace is {}, so the compiled callback raises NameError/Numba errors during setup. Reject values where math.isfinite is false in the converter, as MathExpression._parameters already does.

4. Species names are checked late (simulation.py:1134-1143). An unknown external_field.species is only caught after the FBPIC simulation, particles and diagnostics have been built. The names are already known from the config: density.elec_name, density.ion_name and beam.particle_name. Checking them in the validation step (near simulation.py:418) would fail fast and also cover dry runs.

5. Duplicate species names are silently overwritten. If two densities or beams use the same diagnostic name, self.particle_species[name] = ... keeps the last one, and an external field targeting that name only acts on that species. A duplicate-name check (or a warning) would help. It would also catch a diagnostic-name clash that exists independently of this PR.

6. Docstring workaround. Telling users to target a species without diagnostics via select={"x": [1.0e12, None]} works, but it couples field targeting to diagnostics. Consider a separate, non-diagnostic species label later. That's fine as follow-up work.

---

🟢 Tests

• Diagnostics refactor: behaviour is preserved. The old number_dumps != 0 guard is no longer needed. With number_dumps == 0, regular_diagnostic_kwargs is None, write_final_diagnostic is False and boosted_diagnostic_ranges == [], so _add_particle_diagnostics adds nothing.
• No test compiles a callback through FBPIC/Numba. Every test monkeypatches FBPICExternalField or calls the plain-Python callback. One small test that builds a real ExternalField would catch the Numba-compatibility issues above. Even a CPU-only numba.vectorize of the generated function, for one uniform and one np.exp expression, would do.
• Weak assertion in test_uniform_external_field_builds_one_field_per_component (test_external_field.py:51): in {3.0, 0.5}. Dict order is deterministic, so check each component's value exactly.

---

Summary

Severity	Item
🔴	Parameters named t/x/y/z/gathered_field are silently replaced inside the callback
🟠	Confirm FBPIC's boosted-frame handling of transverse field components
🟡	Numba-compatible subset, SI units in docs, non-finite uniform values, early and duplicate species checks
🟢	Add a test that compiles a real callback; tighten one assertion

I didn't run the test suite because running Python commands needed approval in this environment. This review is from reading the code only.

[joshmann-inversionsemi]

Addressed each of Claude's concerns. FBPIC's documentation indicates that the applied fields are lab frame, implying they are boosted for boosted simulations automatically.

Please provide another review, @claude .

[claude]

Claude finished @joshmann-inversionsemi's task in 2m 13s —— View job

---

Re-review: FBPIC external fields

• Gather context (11 new commits since last review)
• Verify each previous finding was addressed
• Review new code for regressions
• Post review

All of the previous findings have been addressed, and I found no regressions. I have two small new notes and one optional follow-up. Nothing here blocks merge.

---

✅ Previous findings

Previous item	Status	Where
🔴 Parameters with callback-argument names were silently replaced	Fixed. _RESERVED_CALLBACK_PARAMETERS now covers gathered_field, x, y, z, t, amplitude and length_scale, with a parametrized test for each name.	external_field.py:23-25, 62-67
🟠 Boosted-frame transverse fields	Accepted, based on your reading of the FBPIC docs (see the note below).	—
Numba-compatible subset	Fixed. sp, scipy and i are rejected by an AST scan, and the docstring lists what is supported. MathExpression._parameters already stops parameters from shadowing module or constant names, so the namespace from compilation_namespace() can't be hijacked.	external_field.py:68-78, 177-185
SI units in docs	Fixed. The README describes the field as "1 T" and the docstrings no longer call the values dimensionless.	—
Non-finite uniform strengths	Fixed. Rejected with math.isfinite, and tested for nan and ±inf.	external_field.py:40-45
Late species validation	Fixed. Now checked in __attrs_post_init__, so dry runs are covered too. The check at setup time remains as a fallback.	simulation.py:425-448
Duplicate species names	Fixed. particle_species: dict[str, list[Particles]] holds every species that shares a name. build_external_fields creates one FBPIC field per species and component.	simulation.py:349, 1099-1104, 1129-1132
Tests that actually compile	Added. test_external_field_callbacks_compile_with_fbpic_numba uses a real ExternalField and its cpu_func, for both a uniform field and an np.exp expression.	test_external_field.py
Weak in {3.0, 0.5} assertion	Fixed. Values are now checked per component with an exact dict comparison.	test_external_field.py:52-56

The diagnostics refactor still preserves the old behaviour. The number_dumps != 0 guard can still be safely dropped, for the reason given in the last review.

---

🟡 New minor notes

1. An ion_name with no ion species passes the construction check but fails at setup

_configured_particle_species_names() (simulation.py:439-448) accepts density.ion_name without checking whether ions will exist. For an electron-only profile (species=None), _DensityProfile.add_to_simulation returns ion=None (density_core.py:525-546). Setup then never registers ion_name.

Example: SomeProfile(species=None, ion_name="ions") with UniformExternalField(..., species="ions") passes construction. It then raises "not a recorded particle-species name" at simulation.py:1163-1169, after the FBPIC simulation has been built. This is the late failure that item 4 was meant to remove.

The fix is to include ion_name only when density.species is not None. This doesn't arise in boosted runs, because a bare-electron profile is already rejected there.

Fix this →

2. The documented Numba subset is only partly enforced. The docstring says list/tuple literals and np.linalg.norm are unsupported, but _expression_field_mapping only rejects sp, scipy and i. An expression like np.linalg.norm([x, y]) still gets through and fails inside FBPIC's Numba compilation. The docstring is accurate, so this is fine to leave. If you want config-time errors, you could add ast.List/ast.Tuple and np.linalg/numpy.linalg attribute paths to the same AST scan.

---

💬 On boosted frames

I still couldn't read the FBPIC source here, so I'm relying on your reading of the docs, which is reasonable. The remaining question is narrow. Each FBPICExternalField gets a single fieldtype. If FBPIC applies the boost by transforming coordinates and scaling that component only, a lab-frame By would never add its −γβc·By term to Ex'.

This doesn't affect the demo, because Bz is invariant under a boost along z. If you expect transverse external fields in boosted runs, a quick check would settle it: a boosted run with only a lab-frame By should push particles through Ex' as well. If FBPIC doesn't handle this, a one-line docstring caveat would cover it.

---

Optional follow-up (unchanged)

The "impossible select" workaround for targeting an unrecorded species still adds ParticleDiagnostic objects, which write empty particle records at every dump. A separate, non-diagnostic species label would remove that. It's fine as a later change.

---

Verdict: LGTM. Note 1 is a quick fix worth making before merge; everything else is optional. I reviewed by reading the code only. I couldn't run the tests or open the FBPIC source, because running Python and reading outside the repo need approval in this environment.

[joshmann-inversionsemi]

All of Claude's main concerns have been resolved. It is being a bit confused about boosted fields, but FBPIC handles boosting so it's all good.

After this merge, I don't think there are any other classes that will need to be added. Some small gaps will still need to be filled but we should be good to excise lume-fbpic after this merge and its updates.

[moellep]

Looks good