Skip to content

feat: type the Pythia8 boundary with canonical quantities#72

Merged
olantwin merged 1 commit into
mainfrom
feat/units-pythia8
Jul 23, 2026
Merged

feat: type the Pythia8 boundary with canonical quantities#72
olantwin merged 1 commit into
mainfrom
feat/units-pythia8

Conversation

@olantwin

@olantwin olantwin commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Stacked on #71. Beam energies, lifetime thresholds and production times become typed quantities; the hardcoded 299.792458 is replaced by the exact c-derived conversion (differences ≤1 ulp; 200-event validation run bitwise identical).

Summary by CodeRabbit

  • Enhancements
    • Added unit-aware handling for beam energy, target geometry, interaction lengths, particle lifetimes, and position offsets.
    • Configuration values now support explicit units with sensible defaults.
    • Improved consistency of particle timing and position conversions across Pythia8 sources and benchmarks.
  • Bug Fixes
    • Reduced the risk of incorrect simulations caused by ambiguous or mismatched measurement units.
    • Updated Pythia8 plugins and benchmarks to include the required unit support.

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a836134d-1468-4977-be0f-b9307ba07a39

📥 Commits

Reviewing files that changed from the base of the PR and between d98bf5c and cba7262.

📒 Files selected for processing (5)
  • CMakeLists.txt
  • benchmarks/pythia8_benchmark.cpp
  • src/fixed_target_source.cpp
  • src/pythia8_source.cpp
  • src/pythia_common.hpp

📝 Walkthrough

Walkthrough

Changes

Unit-aware Pythia integration

Layer / File(s) Summary
Shared unit-aware Pythia helpers
src/pythia_common.hpp
Pythia helpers now accept SHiP energy, length, and time quantities, converting values at Pythia and MCParticle boundaries.
Unit-aware source configuration
src/pythia8_source.cpp, src/fixed_target_source.cpp
Source constructors and configuration parsing use typed beam, geometry, interaction-length, and lifetime parameters; interaction-point sampling is unit-aware.
Benchmark unit integration and linkage
benchmarks/pythia8_benchmark.cpp, CMakeLists.txt
Benchmark calls use explicit energy and mm/c units, and Pythia plugins plus the benchmark link against SHiP::SHiPUnits.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • ShipSoft/aegir#34: Updates the related Phlex source integration and registration areas.
  • ShipSoft/aegir#38: Modifies the shared Pythia helpers and their source and benchmark callers.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description states the goal but omits the required checklist items from the template. Add the three checklist items for commit message, pre-commit checks, and CI checks, and mark each one.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: making the Pythia8 boundary use canonical quantities.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/units-pythia8

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@olantwin
olantwin force-pushed the feat/units-pythia8 branch from 2952d72 to e9ec269 Compare July 10, 2026 09:28
@olantwin
olantwin force-pushed the feat/units-clhep-bridge branch from c607c48 to 53ed804 Compare July 10, 2026 09:28
@olantwin
olantwin marked this pull request as ready for review July 14, 2026 14:51
@olantwin
olantwin force-pushed the feat/units-clhep-bridge branch from 53ed804 to 8a63cb3 Compare July 14, 2026 16:48
@olantwin
olantwin force-pushed the feat/units-pythia8 branch from e9ec269 to 52e61e0 Compare July 14, 2026 16:48
@olantwin
olantwin force-pushed the feat/units-clhep-bridge branch from 8a63cb3 to 1f6f65f Compare July 22, 2026 13:15
Base automatically changed from feat/units-clhep-bridge to main July 22, 2026 14:04
Pythia8's native units (GeV, mm, mm/c production time) become explicit:
beam energies are ship::Energy, lifetime thresholds and tProd are mm/c
quantities, and the mm/c -> ns conversion derives from the exact SI
definition of c instead of a hand-typed 299.792458. Fixed-target and
Pythia8 source configuration is read via get_quantity, so each
parameter's unit is defined in exactly one place.

The only numeric change is the tProd conversion (division by 299.792458
becomes multiplication by the c-derived factor): at most 1 ulp on ~20%
of values. Validation histograms of a 200-event pythia8_only run are
bitwise identical to an origin/main build.
@olantwin
olantwin force-pushed the feat/units-pythia8 branch from 52e61e0 to cba7262 Compare July 22, 2026 14:14
@github-actions

Copy link
Copy Markdown

Physics Metrics Comparison

1 configuration(s) have differences

  • Configurations compared: 1
  • With differences: 1

Configurations with Differences

comparison_physics-smoke

  • Files compared: 1
  • Histograms compared: 8
  • Fits compared: 0
  • Total differences: 4
Show differences
gun_st_nofield_smoke_validation.root:
  h_hit_edep.integral: 896 → 927 (Δ=3.46%)
  h_hit_z.integral: 896 → 927 (Δ=3.46%)
  h_hit_detector.integral: 896 → 927 (Δ=3.46%)
  h_hit_momentum.integral: 896 → 927 (Δ=3.46%)


About this comparison

Physics metrics are extracted from ROOT files and compared.
Default comparison uses 5% tolerance for histogram statistics
and 3σ for fit parameters.

Metrics stored in git notes: refs/notes/ci/physics-metrics/<config>

@olantwin

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@olantwin

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@olantwin

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@olantwin
olantwin merged commit c63404c into main Jul 23, 2026
7 checks passed
@olantwin
olantwin deleted the feat/units-pythia8 branch July 23, 2026 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant