KFParticle: added local dE/dx file option, removed condition on import of daughter BCOs from GL1#4168
Merged
osbornjd merged 4 commits intosPHENIX-Collaboration:masterfrom Feb 6, 2026
Conversation
added 2 commits
February 5, 2026 19:00
Contributor
📝 WalkthroughWalkthroughThe changes add support for loading dE/dx fit parameters from a local PID parametrization file with charge-independent keys, alongside the existing CDBInterface URL approach. Configuration is controlled by new boolean and string members, with public setters to enable this in the reconstruction workflow. Additionally, BCO calculation is now unconditional rather than trigger-dependent. Changes
Possibly related PRs
✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc (1)
670-676:⚠️ Potential issue | 🔴 CriticalNull-pointer dereference of
gl1packetwhen GL1 data is absent.Both
findNode::getClasscalls (lines 670, 673) can returnnullptr. The previous conditional onm_trigger_info_available(line 676, now commented out) effectively guarded against dereferencing a null pointer. With that guard removed, line 675 will crash if neither"GL1RAWHIT"nor"GL1Packet"nodes are present ontopNode.Please add an explicit null check before dereferencing.
Proposed fix
auto* gl1packet = findNode::getClass<Gl1Packet>(topNode, "GL1RAWHIT"); if (!gl1packet) { gl1packet = findNode::getClass<Gl1Packet>(topNode, "GL1Packet"); } - m_bco = gl1packet->lValue(0, "BCO") + m_calculated_daughter_bunch_crossing[0]; - //m_bco = m_trigger_info_available ? gl1packet->lValue(0, "BCO") + m_calculated_daughter_bunch_crossing[0] : 0; + if (gl1packet) + { + m_bco = gl1packet->lValue(0, "BCO") + m_calculated_daughter_bunch_crossing[0]; + } + else + { + m_bco = 0; + }offline/packages/TrackingDiagnostics/TrkrNtuplizer.cc (1)
859-870:⚠️ Potential issue | 🟠 MajorUnbounded queue growth —
m_rawzdc_hist,m_rawmbd_hist,m_rawmbdv10_hist,m_bco_histare never popped.With the
pop()logic commented out (lines 860–864), these queues grow by one entry per event for the lifetime of the job. For a run with N events that's 4 × 8 × N bytes of monotonically growing memory.Additionally, the rate computation at lines 1247–1249 (
back() - front()) now computes a cumulative average over the full run instead of a rolling 50-event window, which changes the physics meaning ofinforzdc/informbd/informbdv10. If this is intentional, the queue push at lines 780–783 should also be removed and replaced with first/current tracking. If not, restore the pop logic.
Comment on lines
1207
to
1226
| if (m_use_local_PID_file) | ||
| { | ||
| std::cout << "using local" << std::endl; | ||
| // new method is independent of charge | ||
| filefit->GetObject("pi_band",f_pion_plus); | ||
| filefit->GetObject("K_band",f_kaon_plus); | ||
| filefit->GetObject("p_band",f_proton_plus); | ||
| filefit->GetObject("pi_band",f_pion_minus); | ||
| filefit->GetObject("K_band",f_kaon_minus); | ||
| filefit->GetObject("p_band",f_proton_minus); | ||
| } | ||
| else | ||
| { | ||
| filefit->GetObject("f_piband", f_pion_plus); | ||
| filefit->GetObject("f_Kband", f_kaon_plus); | ||
| filefit->GetObject("f_pband", f_proton_plus); | ||
| filefit->GetObject("f_piminus_band", f_pion_minus); | ||
| filefit->GetObject("f_Kminus_band", f_kaon_minus); | ||
| filefit->GetObject("f_pbar_band", f_proton_minus); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing null-checks on TF1 pointers after GetObject — risk of nullptr dereference.
If the local PID file doesn't contain the expected keys ("pi_band", "K_band", "p_band"), GetObject silently leaves the TF1* members as nullptr. The subsequent pidMap entries will then hold null pointers, and the first call to get_dEdx_fitValue → pidMap[PID]->Eval(momentum) will crash.
The pre-existing CDB branch has the same structural risk, but a user-supplied local file is far more likely to have mismatched key names. A defensive check after loading would prevent a silent crash deep in event processing.
Suggested null-check after both branches
}
+ if (!f_pion_plus || !f_kaon_plus || !f_proton_plus ||
+ !f_pion_minus || !f_kaon_minus || !f_proton_minus)
+ {
+ std::cerr << "KFParticle_Tools::init_dEdx_fits: one or more dE/dx fit functions not found in " << dedx_fitparams << std::endl;
+ return;
+ }
+
pidMap.insert(std::pair<int, TF1 *>(-11, f_pion_plus));
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/KFParticle_sPHENIX/KFParticle_Tools.cc (1)
1187-1205:⚠️ Potential issue | 🔴 Critical
TFile::Openreturnsnullptrwhen the file doesn't exist — immediate nullptr dereference on line 1201.
TFile::Openreturns a null pointer if the path is invalid or the file is missing. Line 1201 dereferencesfilefitunconditionally viafilefit->IsOpen(), which will segfault. This was a latent risk with the CDB path, but introducing a user-supplied local filename makes it far more likely to trigger in practice (typos, missing files, wrong working directory, etc.).Proposed fix — guard against nullptr before calling IsOpen
TFile *filefit = TFile::Open(dedx_fitparams.c_str()); - if (!filefit->IsOpen()) + if (!filefit || !filefit->IsOpen()) { std::cerr << "Error opening filefit!" << std::endl; return; }
Build & test reportReport for commit c37ffe92cbd1700573b83a0814d743489bedb407:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 804a7fac2232fbb49fabf43a9f97edc23662e693:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 96944238e5e4cb0feddcfce1627e00d20f1f6db8:
Automatically generated by sPHENIX Jenkins continuous integration |
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.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Currently, KFParticle is only able to use the dE/dx parametrization currently stored in the CDB, which was derived from run 53877; this parametrization is not in general valid for later (or earlier) runs, so until we can repopulate the CDB with run-by-run dE/dx parametrization calibrations, this PR adds an option to use a local file for dE/dx calibration instead.
In addition, this PR removes a condition on extracting the daughter BCOs from the GL1 packet; now they are always retrieved, regardless of the value of
m_trigger_info_available.TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
KFParticle: Local dE/dx file option and GL1 BCO extraction improvements
Motivation & Context
This PR addresses two issues in KFParticle reconstruction:
Key Changes
m_use_local_PID_file(bool, default=false) andm_local_PID_filename(string, default="") member variables to enable loading dE/dx calibrations from local files as an alternative to CDB.useLocalPIDFile(bool)andsetLocalPIDFilename(const std::string&)to KFParticle_sPHENIX for easy configuration.m_trigger_info_availablecondition from BCO calculation in fillBranch()—daughter BCOs are now always retrieved from GL1 packet.dedxfitter/bethe_bloch.hto ensure inline functions are properly defined.Potential Risk Areas
Backward Compatibility
Changes are non-breaking: default behavior (
m_use_local_PID_file=false) maintains original CDB-based dE/dx loading.Possible Future Improvements
Note: This summary is based on AI-generated code analysis and diff descriptions. While reasonable care is taken to ensure accuracy, human judgment is recommended when reviewing the actual code changes, particularly regarding the BCO extraction behavior change and the local file key naming scheme.