feat(data-model): add EventHeader for per-event weight and provenance#17
feat(data-model): add EventHeader for per-event weight and provenance#17matclim wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds ChangesEventHeader integration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Round-trip tests
participant Utils as test::makeEventHeader
participant Writer as TTree/RNTuple writer
participant Storage as ROOT storage
participant Reader as TTree/RNTuple reader
participant Compare as test::equal
Test->>Utils: Create deterministic EventHeader
Utils-->>Writer: Provide eventHeader values
Writer->>Storage: Write eventHeader
Storage-->>Reader: Read eventHeader
Reader->>Compare: Compare with expected EventHeader
Compare-->>Test: Return round-trip result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
| /// Per-event metadata — one record per event | ||
| struct EventHeader { | ||
| double weight{1.0}; ///< Event weight (e.g. P_DIS / nReplicas) | ||
| std::int64_t parentEventId{-1}; ///< Originating event id, e.g. the muon |
There was a problem hiding this comment.
May I suggest renaming this to original_event_id or something like that?
|
|
||
| /// Per-event metadata — one record per event | ||
| struct EventHeader { | ||
| double weight{1.0}; ///< Event weight (e.g. P_DIS / nReplicas) |
There was a problem hiding this comment.
This should be a fixed-width type for portability. E.g. std::float64_t
There was a problem hiding this comment.
Tried it, but std::float64_t turns out not to be usable in a class that needs a ROOT dictionary. rootcling fails outright:
include/SHiP/EventHeader.hpp:10:8: error: no type named 'float64_t' in namespace 'std'
std::float64_t weight{1.0};
~~~~~^
Error: rootcling: compilation failure (libSHiPDataModel..._dictUmbrella.h)
The cause is a compiler asymmetry. libstdc++'s defines std::float64_t only under #ifdef STDCPP_FLOAT64_T. GCC defines that macro, so the ordinary compile succeeds — but rootcling embeds cling, which is Clang-based and doesn't define it, so the header expands to nothing during dictionary generation and the type doesn't exist. Worth noting std::float64_t is also a distinct type (_Float64), not an alias for double, so even past the parse there'd be a second question of whether ROOT's streamer machinery maps it to a basic type at all.
Instead I've kept double and asserted the property directly:
static_assert(std::numeric_limits<double>::is_iec559 && sizeof(double) == 8,
"EventHeader::weight requires IEEE-754 binary64 double");
This should give the same portability guarantee at compile time — the build fails loudly on any platform where double isn't binary64 — without breaking dictionary generation. The rationale is also recorded in a comment in the header so the next person doesn't retry it.
One consistency point worth raising: every other floating-point member in the model is plain double (MCParticle::energy, SimHit::energyDeposit, pathLength, RecParticle::ipPV, and the std::array<double, 3> members), so weight would otherwise be the only fixed-width float. If we do want fixed-width floats across the model, that seems like its own PR, and it would need a ROOT-side solution first.
There was a problem hiding this comment.
You're right, I used double everywhere else. It's always easier to spot these things in other people's code, just like typos...
Could the issue be with C++23 being required for std::float64_t?
For this PR, let's just stick with double, I'll have a look whether we can move all doubles to something of machine-independent size.
Add SHiP::EventHeader, a per-event metadata record carrying an event weight and the id of the originating event. The weight (default 1.0) supports weighted generation such as forced or oversampled muon DIS, where each replica of a source muon is written as its own event and down-weighted accordingly. parentEventId (default -1) records which source event a replica came from — event-level provenance, distinct from MCParticle::motherId, which links particles within an event. The struct is a single per-event record like SimResult, so it is registered in the ROOT dictionary without a std::vector counterpart, and added to both the installed header set and the dictionary inputs. RNTuple and TTree round-trip tests cover it, and the TTree dictionary-presence check now includes SHiP::EventHeader. Purely additive: existing classes and files are unchanged, so old files simply lack the new field.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/SHiP/EventHeader.hpp`:
- Around line 15-22: Rename EventHeader::originalEventId to the contracted
parentEventId and update every producer, consumer, helper, and serialization
round-trip expectation to use the new member consistently. Preserve the existing
default value and provenance semantics while ensuring no references to
originalEventId remain in the public API or related schema handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 683cbe87-70db-437f-8ce7-7e36f55d613f
📒 Files selected for processing (7)
CMakeLists.txtREADME.mdinclude/SHiP/EventHeader.hppinclude/SHiP/LinkDef.htests/test_rntuple_io.cpptests/test_ttree_io.cpptests/test_utils.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- include/SHiP/LinkDef.h
- tests/test_utils.hpp
- CMakeLists.txt
- README.md
- tests/test_ttree_io.cpp
| /// Per-event metadata — one record per event | ||
| struct EventHeader { | ||
| double weight{1.0}; ///< Event weight (e.g. P_DIS / nReplicas) | ||
| std::int64_t originalEventId{-1}; ///< Originating event id, e.g. the muon | ||
| ///< event that seeded this replica | ||
| ///< (-1 = none). Event-level provenance, | ||
| ///< distinct from MCParticle::motherId, | ||
| ///< which links particles within an event. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the contracted parentEventId member name.
The PR contract specifies parentEventId, but this public record exposes originalEventId. Rename the member and update all producers, consumers, helpers, and round-trip expectations consistently; otherwise downstream code and serialized schemas will observe a different API.
Proposed fix
- std::int64_t originalEventId{-1};
+ std::int64_t parentEventId{-1};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/SHiP/EventHeader.hpp` around lines 15 - 22, Rename
EventHeader::originalEventId to the contracted parentEventId and update every
producer, consumer, helper, and serialization round-trip expectation to use the
new member consistently. Preserve the existing default value and provenance
semantics while ensuring no references to originalEventId remain in the public
API or related schema handling.
2 participants
Add SHiP::EventHeader, a per-event metadata record carrying an event weight and the id of the originating event.
The weight (default 1.0) supports weighted generation such as forced or oversampled muon DIS, where each replica of a source muon is written as its own event and down-weighted accordingly. originalEventId (default -1) records which source event a replica came from — event-level provenance, distinct from MCParticle::motherId, which links particles within an event.
The struct is a single per-event record like SimResult, so it is registered in the ROOT dictionary without a std::vector counterpart, and added to both the installed header set and the dictionary inputs. RNTuple and TTree round-trip tests cover it, and the TTree dictionary-presence check now includes SHiP::EventHeader. Purely additive: existing classes and files are unchanged, so old files simply lack the new field.
Summary by CodeRabbit
New Features
SHiP::EventHeaderto store per-event metadata, including eventweightandoriginalEventIdprovenance.Documentation
SHiP::EventHeaderunder event metadata.Tests
EventHeadervalues correctly.