From 64da97717b9cb4cbc9aa76db57168eb62fb6663e Mon Sep 17 00:00:00 2001 From: Oliver Lantwin Date: Thu, 9 Jul 2026 14:18:46 +0200 Subject: [PATCH 1/2] feat(lint): fence unit conventions with prek hooks Four pygrep fences guard the unit rules established in the preceding changes: no hardcoded speed of light, no G4SystemOfUnits.hh (its unqualified globals collide with the mp-units vocabulary), no CLHEP unit constants outside src/units/, and no hand-typed power-of-ten conversion factors. Verified against pre-migration origin/main: the hooks flag exactly the historical conversion sites. Also documents the conventions in docs/units.md and annotates every dimensional default in workflows/lib.libsonnet with its unit. --- .pre-commit-config.yaml | 35 ++++++++++++++++++++++++ docs/units.md | 59 +++++++++++++++++++++++++++++++++++++++++ workflows/lib.libsonnet | 36 ++++++++++++------------- 3 files changed, 112 insertions(+), 18 deletions(-) create mode 100644 docs/units.md diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2825a27..35b3c4f 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -17,6 +17,41 @@ repos: - id: check-merge-conflict - id: mixed-line-ending + # Unit-safety fences (see docs/units.md). pygrep hooks fail on any match. + - repo: local + hooks: + - id: forbid-hardcoded-c + name: forbid hardcoded speed of light + language: pygrep + # Digit-anchored so it does not fire inside a longer number (e.g. a + # seed like 12997924580 that merely contains the c digits). + entry: '(? or "to"). The comment + # marker keeps it from firing on unrelated powers of ten. + entry: '(?|\bto\b)' + types_or: [c++, c] + # Tools below come from the pixi `lint` environment via language: system. - repo: local hooks: diff --git a/docs/units.md b/docs/units.md new file mode 100644 index 0000000..cd59282 --- /dev/null +++ b/docs/units.md @@ -0,0 +1,59 @@ + + +# Unit handling + +FairShip was plagued by unit-mismatch bugs; aegir prevents them +structurally. The rules: + +## Canonical units and types + +The framework-wide vocabulary lives in `SHiP/Units.hpp` (shipdatamodel, +`SHiP::SHiPUnits` target), built on [mp-units](https://mpusz.github.io/mp-units/): + +| Quantity | Type | Storage unit | +|-----------------|-----------------------|--------------| +| Length/position | `ship::Length` | mm | +| Time | `ship::Time` | ns | +| Energy | `ship::Energy` | GeV | +| Momentum | `ship::Momentum` | GeV/c | +| Mass | `ship::Mass` | GeV/c² | +| Angle | `ship::Angle` | rad | +| Magnetic field | `ship::MagneticField` | T | + +`GeV/c` and `mm/c` are real derived units built from the exact SI +definition of c, so `energy + momentum` is a compile error and +conversions such as Pythia's `tProd()` (mm/c → ns) are derived rather +than hand-typed. Persisted data-model structs stay plain doubles in the +storage units (ROOT cannot stream quantity types); `SHiP/QuantityView.hpp` +provides typed views over them. + +## The one rule at boundaries + +**A raw double acquires its unit on the line it enters, and only leaves +a quantity via an explicit conversion.** + +- Geant4/CLHEP: only through `src/units/clhep_bridge.hpp` — the single + file allowed to reference CLHEP unit constants. `G4SystemOfUnits.hh` + is banned repo-wide (it injects global `mm`, `GeV`, `tesla`, ... that + collide with the mp-units vocabulary). +- Pythia8 (GeV, mm, mm/c), GENIE (SI): values are wrapped at the read + site (`p.tProd() * su::mm_per_c`, `vtx[0] * su::m`). +- Configuration: workflow files stay plain numbers; C++ reads them with + `aegir::get_quantity(config, "beam_energy", 400.0 * su::GeV)`, making + the parameter definition the single source of truth for the unit. + +Never `using namespace ship::units` in a translation unit that includes +CLHEP or Geant4 headers — always qualify. + +## Enforcement + +prek hooks (see `.pre-commit-config.yaml`) fail on: hardcoded +299792458/299.792458, any `G4SystemOfUnits.hh` include, `CLHEP/Units/` +outside `src/units/`, and `1e3`/`1e9`-style factors next to unit +conversion comments. Review rule: no raw double crosses a function +boundary with an implicit unit — either the parameter is a quantity +type, or the conversion happens on the line the value is read. diff --git a/workflows/lib.libsonnet b/workflows/lib.libsonnet index c27cfe4..b5919ba 100644 --- a/workflows/lib.libsonnet +++ b/workflows/lib.libsonnet @@ -40,38 +40,38 @@ gun:: { cpp: 'particle_gun_source', pdg: 13, - p_min: 10.0, - p_max: 100.0, - max_theta: 0.1, - vertex_z: -500.0, + p_min: 10.0, // GeV/c + p_max: 100.0, // GeV/c + max_theta: 0.1, // rad + vertex_z: -500.0, // mm }, // Pencil beam for the field smokes: straight 20 GeV μ− from z = −2 m. pencil_gun:: { cpp: 'particle_gun_source', pdg: 13, - p_min: 20.0, - p_max: 20.0, - max_theta: 0.0, - vertex_z: -2000.0, + p_min: 20.0, // GeV/c + p_max: 20.0, // GeV/c + max_theta: 0.0, // rad + vertex_z: -2000.0, // mm }, pythia8:: { cpp: 'pythia8_source', - beam_energy: 400.0, + beam_energy: 400.0, // GeV process: 'SoftQCD:inelastic', }, fixed_target:: { cpp: 'fixed_target_source', - beam_energy: 400.0, - target_z: 74, + beam_energy: 400.0, // GeV + target_z: 74, // proton number, not a coordinate target_a: 184, // Target z extents to be determined from GeoModel geometry - target_z_start: 0.0, - target_z_end: 1164.0, - interaction_length: 191.9, - tau0_threshold: 1.0, + target_z_start: 0.0, // mm + target_z_end: 1164.0, // mm + interaction_length: 191.9, // mm + tau0_threshold: 1.0, // mm/c }, // Neutrino interactions pre-generated with GENIE, read from a rootracker @@ -95,10 +95,10 @@ cpp: 'geant4_module', physics_list: 'FTFP_BERT', sd_mode: 'crossing', - ke_threshold: 0.5, + ke_threshold: 0.5, // GeV energy_cut: true, - particle_ke_cut: 1.0, - regions: { '/SHiP/target': 50, '/SHiP/muon_shield/magn_absorb': 50 }, + particle_ke_cut: 1.0, // GeV + regions: { '/SHiP/target': 50, '/SHiP/muon_shield/magn_absorb': 50 }, // production cuts [mm] }, noop_output:: { cpp: 'sim_output_module', mode: 'noop' }, From f7e480ec31f58091503953cee19557c966cd4b3a Mon Sep 17 00:00:00 2001 From: Oliver Lantwin Date: Fri, 24 Jul 2026 14:38:12 +0200 Subject: [PATCH 2/2] fix(lint): align unit fences with the documented policy The prek fences were looser than docs/units.md already claims. Match them up and close notation gaps flagged in review: - forbid-g4-system-of-units: match the #include directive and drop the src/units/ exclusion, so the ban is repo-wide as documented (the bridge's prose mention of the header no longer needs a carve-out). - forbid-clhep-units-outside-bridge: narrow the exception to clhep_bridge.hpp (the only file that needs CLHEP) and also catch CLHEP:: symbol use, not just the include path. - forbid-hardcoded-c: also match scientific-notation (2.99792458e8) and digit-separated forms of the speed of light. - forbid-magic-unit-factors: accept signed-positive exponents (1e+3) and single-line block-comment conversion markers. --- .pre-commit-config.yaml | 27 +++++++++++++++------------ docs/units.md | 9 +++++---- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 35b3c4f..c18c216 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -24,32 +24,35 @@ repos: name: forbid hardcoded speed of light language: pygrep # Digit-anchored so it does not fire inside a longer number (e.g. a - # seed like 12997924580 that merely contains the c digits). - entry: '(?"]*G4SystemOfUnits\.hh' types_or: [c++, c] - # The bridge's header comment documents this very ban. - exclude: ^src/units/ - id: forbid-clhep-units-outside-bridge - name: forbid CLHEP units outside src/units/ + name: forbid CLHEP units outside the bridge language: pygrep - entry: 'CLHEP/Units/' + # Include path or CLHEP:: symbols; the bridge is the one file allowed both. + entry: 'CLHEP/Units/|\bCLHEP::' types_or: [c++, c] - exclude: ^src/units/ + exclude: '^src/units/clhep_bridge\.hpp$' - id: forbid-magic-unit-factors name: forbid hand-typed power-of-ten unit conversions language: pygrep - # Any 1e{3,6,9}-style factor (decimal mantissa and f/l suffixes - # included) next to a conversion comment (-> or "to"). The comment - # marker keeps it from firing on unrelated powers of ten. - entry: '(?|\bto\b)' + # Any 1e{3,6,9}-style factor (signed exponent, decimal mantissa and + # f/l suffixes included) next to a conversion comment (-> or "to", in + # a // or /* */ comment). The marker keeps it from firing on unrelated + # powers of ten. + entry: '(?|\bto\b)' types_or: [c++, c] # Tools below come from the pixi `lint` environment via language: system. diff --git a/docs/units.md b/docs/units.md index cd59282..6a21fe9 100644 --- a/docs/units.md +++ b/docs/units.md @@ -51,9 +51,10 @@ CLHEP or Geant4 headers — always qualify. ## Enforcement -prek hooks (see `.pre-commit-config.yaml`) fail on: hardcoded -299792458/299.792458, any `G4SystemOfUnits.hh` include, `CLHEP/Units/` -outside `src/units/`, and `1e3`/`1e9`-style factors next to unit -conversion comments. Review rule: no raw double crosses a function +prek hooks (see `.pre-commit-config.yaml`) fail on: hardcoded c +(299792458, 299.792458 or 2.99792458e8, digit separators allowed), any +repo-wide `G4SystemOfUnits.hh` include, `CLHEP/Units/` includes or +`CLHEP::` symbols outside `src/units/clhep_bridge.hpp`, and +`1e±3`/`1e±9`-style factors next to unit conversion comments. Review rule: no raw double crosses a function boundary with an implicit unit — either the parameter is a quantity type, or the conversion happens on the line the value is read.