Add RecParticle - #2
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a new plain data struct ChangesRecParticle data class integration
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/RecParticle.hpp`:
- Line 34: Fix the typo in the trailing comment for the member variable ipPV in
RecParticle (double ipPV{0};) by removing the extraneous '>' so the unit
annotation reads "[mm]" instead of "[mm]>" and ensure the comment remains "///<
IP wrt to the PV (at 0,0,0) [mm]".
- Around line 10-35: RecParticle currently hides all data members making it
unusable; update the declaration so external code can read reconstruction
results by either converting class RecParticle to a struct or making its member
variables public (trackId, parentId, pdgCode, vertex, endpoint, momentum,
energy, time, creatorProcess, ipPV), or alternatively add const accessors for
each field (e.g., trackId(), parentId(), pdgCode(), vertex(), endpoint(),
momentum(), energy(), time(), creatorProcess(), ipPV()) and keep the existing
constructor RecParticle(const SimParticle&). Ensure the chosen change is applied
consistently to the existing constructor and any users of RecParticle.
- Line 3: Replace the incorrect include in RecParticle.hpp from
<SimParticle.hpp> to the project SHiP path (e.g., "SHiP/SimParticle.hpp" or
<SHiP/SimParticle.hpp>) so the header is found via BASE_DIRS; remove the stray
extra '>' in the comment near the parameter units (the comment around line 34
that currently reads "... [mm]>"); and make the reconstructed-data members of
class RecParticle accessible by adding appropriate public getters (or making
them public) so external C++ code can read the reconstructed values rather than
relying solely on ROOT I/O or reflection.
🪄 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: 376e78e2-392a-4254-a3e2-994e352fe2d2
📒 Files selected for processing (3)
CMakeLists.txtinclude/SHiP/LinkDef.hinclude/SHiP/RecParticle.hpp
Contributor
Author
|
There is a clash in the pre-commit ci. Something is putting the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/SHiP/RecParticle.hpp (1)
23-36: ⚡ Quick winAdd documentation and consider
noexceptfor the converter function.The
fromSimParticlefunction lacks documentation explaining its purpose, what happens toipPV, and why it's a free function rather than a constructor. Additionally, since the function only copies POD-like data without allocations or throwing operations, marking itnoexceptwould help compilers optimize and clarify the exception guarantee.📚 Suggested documentation and noexcept addition
+/// Converts a SimParticle to RecParticle +/// \param sp Source simulation particle +/// \return RecParticle with copied fields and ipPV initialized to 0.0 -inline RecParticle fromSimParticle(SimParticle const& sp) { +inline RecParticle fromSimParticle(SimParticle const& sp) noexcept { return { .trackId = sp.trackId,🤖 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/RecParticle.hpp` around lines 23 - 36, Add a brief doc comment above the free function fromSimParticle explaining its purpose (convert a SimParticle to a RecParticle), what ipPV is initialized to (explicitly set to 0.0 here) and why this conversion is provided as a free function rather than a RecParticle constructor (e.g., non-owning conversion helper, keeps RecParticle lightweight, or to avoid dependency on SimParticle in RecParticle header). Also mark the function noexcept since it only performs trivial member copies of POD-like fields (update the signature inline RecParticle fromSimParticle(SimParticle const& sp) noexcept) to document the non-throwing guarantee and enable compiler optimizations.
🤖 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/RecParticle.hpp`:
- Around line 3-5: The include order in RecParticle.hpp is wrong: move the
standard library headers <array> and <cstdint> so they come before the project
header <SHiP/SimParticle.hpp> to match the convention used across other SHiP
headers; update the top-of-file include sequence so that <array> and <cstdint>
appear first, followed by <SHiP/SimParticle.hpp> (the file referencing
RecParticle/SimParticle symbols) to satisfy the include-order lint.
---
Nitpick comments:
In `@include/SHiP/RecParticle.hpp`:
- Around line 23-36: Add a brief doc comment above the free function
fromSimParticle explaining its purpose (convert a SimParticle to a RecParticle),
what ipPV is initialized to (explicitly set to 0.0 here) and why this conversion
is provided as a free function rather than a RecParticle constructor (e.g.,
non-owning conversion helper, keeps RecParticle lightweight, or to avoid
dependency on SimParticle in RecParticle header). Also mark the function
noexcept since it only performs trivial member copies of POD-like fields (update
the signature inline RecParticle fromSimParticle(SimParticle const& sp)
noexcept) to document the non-throwing guarantee and enable compiler
optimizations.
🪄 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: 776c2c23-1c5b-4826-ace6-e7ee46f53fcb
📒 Files selected for processing (2)
include/SHiP/LinkDef.hinclude/SHiP/RecParticle.hpp
💤 Files with no reviewable changes (1)
- include/SHiP/LinkDef.h
Comment on lines
+3
to
+5
| #include <SHiP/SimParticle.hpp> | ||
| #include <array> | ||
| #include <cstdint> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check include order patterns in existing SHiP headers
echo "== Include order in other SHiP headers =="
for f in include/SHiP/*.hpp; do
if [[ "$f" == *"RecParticle.hpp"* ]]; then continue; fi
echo "--- $f ---"
rg -n '^`#include`' "$f" | head -n 10
done
echo ""
echo "== Check if pre-commit config specifies include order =="
fd -t f -e yaml -e yml -e toml -e cfg 'pre-commit|clang-format|cpplint' . | head -n 5 | while read cfg; do
echo "--- $cfg ---"
cat "$cfg" | head -n 100
doneRepository: ShipSoft/data-model
Length of output: 659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pre-commit config files =="
fd -t f -e yml -e yaml '.pre-commit-config*' . || true
fd -t f -e cfg -e toml '.clang-format|CPPLINT|cpplint' . || true
echo
echo "== CPPLINT config =="
ls -1 CPPLINT.cfg 2>/dev/null || true
[ -f CPPLINT.cfg ] && cat CPPLINT.cfg
echo
echo "== Include-order scan (first few includes per header) =="
for f in include/SHiP/*.hpp; do
echo "--- $f ---"
# show first non-empty include lines
awk '{
if ($0 ~ /^`#include/`){ print NR ":" $0; n++ }
else if(n>=1 && $0 ~ /^[[:space:]]*$/){ next }
else if(n>=1 && $0 !~ /^[[:space:]]*$/){ exit }
}' "$f"
doneRepository: ShipSoft/data-model
Length of output: 784
Fix include order in include/SHiP/RecParticle.hpp
include/SHiP/RecParticle.hpp (line 3) includes <SHiP/SimParticle.hpp> before <array>/<cstdint>, while other include/SHiP/*.hpp headers place standard library includes first (and include/SHiP/SimResult.hpp places "SHiP/..." after <vector>). Reorder to match the established convention to align with the include-order CI/lint expectations.
🤖 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/RecParticle.hpp` around lines 3 - 5, The include order in
RecParticle.hpp is wrong: move the standard library headers <array> and
<cstdint> so they come before the project header <SHiP/SimParticle.hpp> to match
the convention used across other SHiP headers; update the top-of-file include
sequence so that <array> and <cstdint> appear first, followed by
<SHiP/SimParticle.hpp> (the file referencing RecParticle/SimParticle symbols) to
satisfy the include-order lint.
Contributor
Author
|
@olantwin Can we merge this. It is a bit of a placeholder for now but we can extend it later. |
Contributor
|
Apologies, I didn't notice that I wasn't (yet) subscribed to notifications for this repo. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
RecParticleclass.This is to store more information from a reconstructed particle. For now the IP wrt some notional vertex.
Summary by CodeRabbit