Reviewer comments

Author: chillenzer

lib/python/picongpu/picmi/simulation.py

@@ -369,7 +369,7 @@ def get_as_pypicongpu(self) -> pypicongpu.simulation.Simulation:
         typical_ppc = (
             self.picongpu_typical_ppc
             if self.picongpu_typical_ppc is not None
-            else mid_window(map(lambda op: op.layout.ppc, filter(lambda op: hasattr(op, "layout"), init_operations)))
+            else _mid_window(map(lambda op: op.layout.ppc, filter(lambda op: hasattr(op, "layout"), init_operations)))
         )
         moving_window = (
             None
@@ -402,23 +402,6 @@ def get_as_pypicongpu(self) -> pypicongpu.simulation.Simulation:
         )
 
     def _get_base_density(self) -> float:
-        # There's supposed to be some heuristics here along the lines of
-        #        num_grid = (
-        #            np.reshape([grid.cell_size_x_si, grid.cell_size_y_si, grid.cell_size_z_si], (-1, 1, 1, 1))
-        #            * np.mgrid[: grid.cell_cnt_x, : grid.cell_cnt_y, : grid.cell_cnt_z]
-        #        )
-        #        return float(
-        #            np.max(
-        #                np.fromiter(
-        #                    (
-        #                        operation.profile(*num_grid)
-        #                        for operation in self.all_operations
-        #                        if isinstance(operation, SimpleDensity)
-        #                    ),
-        #                    dtype=float,
-        #                )
-        #            )
-        #        )
         return self.picongpu_base_density or 1.0e25
 
     def picongpu_run(self) -> None:
@@ -456,6 +439,14 @@ def organise_init_operations(operations):
     return [run_construction(op) for op in cleaned]
 
 
-def mid_window(iterable):
-    mi, ma = reduce(lambda lhs, rhs: (min(lhs[0], rhs), max(lhs[1], rhs)), iterable, (1000, 0))
-    return (ma - mi) // 2 + mi
+def _mid_window(iterable):
+    """Compute the integer in the middle between min(iterable), max(iterable), return 1 if empty."""
+    iterable = iter(iterable)
+
+    try:
+        start = next(iterable)
+    except StopIteration:
+        return 1
+
+    mi, ma = reduce(lambda lhs, rhs: (min(lhs[0], rhs), max(lhs[1], rhs)), iterable, (start, start))
+    return int((ma - mi) // 2 + mi)

lib/python/picongpu/picmi/species.py

@@ -9,7 +9,7 @@
 import re
 from typing import Any
 
-from pydantic import BaseModel, PrivateAttr, computed_field, model_validator
+from pydantic import BaseModel, PrivateAttr, computed_field, model_validator, field_validator
 
 from picongpu.picmi.distribution import AnyDistribution
 from picongpu.picmi.species_requirements import resolving_add, evaluate_requirements, run_construction
@@ -54,7 +54,7 @@ class PusherMethod(Enum):
 
 
 class Species(BaseModel):
-    name: str | None = None
+    name: str
     particle_type: str | None = None
     initial_distribution: AnyDistribution | None = None
     picongpu_fixed_charge: bool = False
@@ -71,21 +71,22 @@ class Species(BaseModel):
     # For now, we add them to all species. Refinements might be necessary in the future.
     _requirements: list[Any] = PrivateAttr(default_factory=lambda: [Position(), Weighting(), Momentum()])
 
+    @field_validator("name", mode="before")
+    @classmethod
+    def _validate_name(cls, value, values):
+        if value is None:
+            if values["particle_type"] is None:
+                raise ValueError(
+                    "Can't come up with a proper name for your species because neither name nor particle type are given."
+                )
+            value = values["particle_type"]
+        return value
+
     class Config:
         arbitrary_types_allowed = True
 
     @model_validator(mode="after")
     def check(self):
-        if self.name is None and self.particle_type is None:
-            raise ValueError(
-                "Can't come up with a proper name for your species because neither name nor particle type are given."
-            )
-        if self.name is None:
-            self.name = self.particle_type
-        try:
-            is_element = self.particle_type is not None and Element.is_element(self.particle_type)
-        except ValueError:
-            is_element = False
         if self.particle_type is None:
             assert self.charge_state is None, (
                 f"Species {self.name} specified initial charge state via charge_state without also specifying particle "
@@ -94,7 +95,8 @@ def check(self):
             assert self.picongpu_fixed_charge is False, (
                 f"Species {self.name} specified fixed charge without also specifying particle_type"
             )
-        elif is_element:
+        # Returns None if it is not an element, so is False-y in those cases, and True-y otherwise:
+        elif self.picongpu_element:
             if self.charge_state is not None:
                 assert Element(self.particle_type).get_atomic_number() >= self.charge_state, (
                     f"Species {self.name} intial charge state is unphysical"

lib/python/picongpu/picmi/species_requirements.py

@@ -261,8 +261,8 @@ def __init__(self, /, species):
         def constructor(self):
             species = self.metadata.kwargs["species"].get_as_pypicongpu()
             particle_mass_si = species.get_constant_by_type(Mass).mass_si
-            rms_velocity_si_squared = np.linalg.norm(self.metadata.kwargs["rms_velocity"]) ** 2 / 3
-            temperature_kev = particle_mass_si * rms_velocity_si_squared * electron_volt**-1 * 10**-3
+            rms_velocity_si_squared = np.linalg.norm(self.metadata.kwargs["rms_velocity"]) ** 2
+            temperature_kev = particle_mass_si * rms_velocity_si_squared / 3 * electron_volt**-1 * 10**-3
             temperature = Temperature(temperature_kev=temperature_kev) if temperature_kev > 0 else None
             return SimpleMomentum(species=species, drift=self.metadata.kwargs["drift"], temperature=temperature)
 

test/python/picongpu/quick/picmi/species.py

@@ -20,34 +20,29 @@
 from picongpu.pypicongpu.species.constant.mass import Mass
 
 
-def subset_of(subset, superset):
-    superset = list(superset)
-    return all(any(e1 == e2 for e2 in superset) for e1 in subset)
-
-
-def count_all_one(subset, superset):
-    superset = list(superset)
-    return all(superset.count(e) == 1 for e in subset)
+def unique_in(elements, collection):
+    collection = list(collection)
+    return (collection.count(e) == 1 for e in elements)
 
 
 class TestSpeciesRequirementResolution(TestCase):
     def test_deduplicate_attributes(self):
         species = Species(name="dummy")
         requirements = [Weighting()]
         species.register_requirements(2 * requirements)
-        assert count_all_one(requirements, species.get_as_pypicongpu().attributes)
+        assert all(unique_in(requirements, species.get_as_pypicongpu().attributes))
 
     def test_deduplicate_constants(self):
         species = Species(name="dummy")
         requirements = [Mass(mass_si=1.0)]
         species.register_requirements(2 * requirements)
-        assert count_all_one(requirements, species.get_as_pypicongpu().constants)
+        assert all(unique_in(requirements, species.get_as_pypicongpu().constants))
 
     def test_deduplicate_delayed_construction(self):
         species = Species(name="dummy", particle_type="H", charge_state=1)
         requirements = [SetChargeStateOperation(species)]
         species.register_requirements(2 * requirements)
-        assert count_all_one(requirements, species.get_operation_requirements())
+        assert all(unique_in(requirements, species.get_operation_requirements()))
 
     def test_conflicting_constants(self):
         species = Species(name="dummy")