dEdxFitter added to TPC calibrations#4114
dEdxFitter added to TPC calibrations#4114osbornjd merged 4 commits intosPHENIX-Collaboration:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a TPC dE/dx fitting subsystem: a Bethe‑Bloch model header, a self‑contained GlobaldEdxFitter (header+impl), a SubsysReco dEdxFitter module for event/track processing, Autotools build scripts, and analysis macros for sample‑size studies and visualization. Changes
Sequence DiagramsequenceDiagram
participant Event as Reconstruction (Event Loop)
participant dEdxFitter as dEdxFitter (SubsysReco)
participant Fitter as GlobaldEdxFitter (Fitting Engine)
participant Physics as Bethe‑Bloch (Model)
participant ROOT as ROOT (Minimizer)
Event->>dEdxFitter: process_event(topNode)
activate dEdxFitter
dEdxFitter->>dEdxFitter: GetNodes (Track/Cluster/Geometry)
dEdxFitter->>dEdxFitter: process_tracks (iterate SvtxTrackMap)
loop for each track passing cuts
dEdxFitter->>dEdxFitter: get_dedx(track)
dEdxFitter->>Fitter: addTrack(dEdx, p)
activate Fitter
Fitter->>Fitter: store (p, dEdx)
deactivate Fitter
end
alt accumulated tracks > threshold
dEdxFitter->>Fitter: get_minimum() / get_minimum_new()
activate Fitter
Fitter->>ROOT: run minimizer (TF1/TF2/TF3 / Minuit2)
ROOT->>Physics: evaluate bethe_bloch wrapper
Physics-->>ROOT: dE/dx(betagamma, params)
ROOT-->>Fitter: optimized parameters / minimum
Fitter-->>dEdxFitter: return minimum
deactivate Fitter
dEdxFitter->>dEdxFitter: store minimum, reset fitter
end
deactivate dEdxFitter
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
calibrations/tpc/dEdx/GlobaldEdxFitter.hcalibrations/tpc/dEdx/Makefile.amcalibrations/tpc/dEdx/autogen.shcalibrations/tpc/dEdx/bethe_bloch.hcalibrations/tpc/dEdx/configure.accalibrations/tpc/dEdx/dEdxFitter.cccalibrations/tpc/dEdx/dEdxFitter.hcalibrations/tpc/dEdx/test_sample_size.C
🧰 Additional context used
📓 Path-based instructions (3)
**/*.C
⚙️ CodeRabbit configuration file
Do NOT review these files
Files:
calibrations/tpc/dEdx/test_sample_size.C
**/*.{h,hpp,hxx,hh}
⚙️ CodeRabbit configuration file
**/*.{h,hpp,hxx,hh}: Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete.
If interfaces change, ask for compatibility notes and any needed downstream updates.Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.
Files:
calibrations/tpc/dEdx/dEdxFitter.hcalibrations/tpc/dEdx/GlobaldEdxFitter.hcalibrations/tpc/dEdx/bethe_bloch.h
**/*.{cc,cpp,cxx,c}
⚙️ CodeRabbit configuration file
**/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
Flag hidden global state, non-const singletons, and unclear lifetime assumptions.Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.
Files:
calibrations/tpc/dEdx/dEdxFitter.cc
🧬 Code graph analysis (2)
calibrations/tpc/dEdx/dEdxFitter.cc (2)
calibrations/tpc/dEdx/dEdxFitter.h (1)
dEdxFitter(19-86)simulation/g4simulation/g4eval/SvtxHitEval.h (1)
_clustermap(84-84)
calibrations/tpc/dEdx/GlobaldEdxFitter.h (1)
calibrations/tpc/dEdx/bethe_bloch.h (3)
bethe_bloch_new_1D(48-53)dedx_constants(6-29)bethe_bloch_total(66-71)
🪛 Clang (14.0.6)
calibrations/tpc/dEdx/dEdxFitter.h
[error] 4-4: 'fun4all/SubsysReco.h' file not found
(clang-diagnostic-error)
calibrations/tpc/dEdx/bethe_bloch.h
[error] 4-4: 'TMath.h' file not found
(clang-diagnostic-error)
🪛 Shellcheck (0.11.0)
calibrations/tpc/dEdx/autogen.sh
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (2)
calibrations/tpc/dEdx/Makefile.am (1)
1-49: Build configuration looks good.The Makefile.am correctly installs the public headers, builds the shared library, and provides a test harness. No critical issues found.
calibrations/tpc/dEdx/configure.ac (1)
1-16: Autotools configuration is standard and appropriate.The configure.ac follows typical patterns for sPHENIX packages. Enabling
-Wall -Werroris good practice for catching issues early.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| (cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share;\ | ||
| libtoolize --force; automake -a --add-missing; autoconf) |
There was a problem hiding this comment.
Handle cd failure to avoid running autotools in wrong directory
If cd $srcdir fails (e.g., directory doesn't exist), the script continues and runs aclocal, libtoolize, etc. in the wrong location, potentially corrupting the build or producing confusing errors.
Proposed fix
-(cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share;\
-libtoolize --force; automake -a --add-missing; autoconf)
+(cd "$srcdir" || exit 1; aclocal -I "${OFFLINE_MAIN}/share" &&
+ libtoolize --force && automake -a --add-missing && autoconf)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share;\ | |
| libtoolize --force; automake -a --add-missing; autoconf) | |
| (cd "$srcdir" || exit 1; aclocal -I "${OFFLINE_MAIN}/share" &&\ | |
| libtoolize --force && automake -a --add-missing && autoconf) |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
| TF3* GlobaldEdxFitter::create_TF3_new(std::string name) | ||
| { | ||
| TF3* f = new TF3(name.c_str(),this,&GlobaldEdxFitter::get_fitquality_wrapper_new,min_norm,max_norm,min_B,max_B,min_ZS,max_ZS,0,"GlobaldEdxFitter","get_fitquality_wrapper_new"); | ||
| return f; | ||
| } | ||
|
|
||
| TF1* GlobaldEdxFitter::create_TF1(std::string name) | ||
| { | ||
| TF1* f = new TF1(name.c_str(),this,&GlobaldEdxFitter::get_fitquality_wrapper_new,min_norm,max_norm,0,"GlobaldEdxFitter","get_fitquality_wrapper"); | ||
| return f; | ||
| } | ||
|
|
||
| TF2* GlobaldEdxFitter::create_TF2(std::string name) | ||
| { | ||
| TF2* f = new TF2(name.c_str(),this,&GlobaldEdxFitter::get_fitquality_wrapper_ZS,min_norm,max_norm,min_ZS,max_ZS,0,"GlobaldEdxFitter","get_fitquality_wrapper_ZS"); | ||
| return f; | ||
| } |
There was a problem hiding this comment.
Clarify ownership: raw new TF1/TF2/TF3 returns
create_TF1, create_TF2, and create_TF3_new return raw new-allocated ROOT objects. The caller must remember to delete them (or rely on ROOT's ownership model if added to a directory). Consider documenting ownership expectations or returning std::unique_ptr if ROOT ownership is not intended.
| ROOT::Math::Minimizer* minimizer = ROOT::Math::Factory::CreateMinimizer("Minuit2"); | ||
| minimizer->SetMaxFunctionCalls(1000000); | ||
| minimizer->SetMaxIterations(10000); | ||
| minimizer->SetTolerance(0.1); | ||
| minimizer->SetPrintLevel(1); | ||
| ROOT::Math::Functor f(this,&GlobaldEdxFitter::get_fitquality_functor,1); | ||
| double step[1] = {.01}; | ||
| double variable[1] = {20.}; | ||
| minimizer->SetFunction(f); | ||
| minimizer->SetVariable(0,"A",variable[0],step[0]); | ||
| minimizer->Minimize(); | ||
| const double *xs = minimizer->X(); | ||
| return xs[0]; | ||
| } |
There was a problem hiding this comment.
Memory leak: minimizer is never deleted
ROOT::Math::Factory::CreateMinimizer returns an owning raw pointer. The minimizer is never freed, leaking memory on every call to get_minimum_new().
Proposed fix using unique_ptr
- ROOT::Math::Minimizer* minimizer = ROOT::Math::Factory::CreateMinimizer("Minuit2");
+ std::unique_ptr<ROOT::Math::Minimizer> minimizer(
+ ROOT::Math::Factory::CreateMinimizer("Minuit2"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ROOT::Math::Minimizer* minimizer = ROOT::Math::Factory::CreateMinimizer("Minuit2"); | |
| minimizer->SetMaxFunctionCalls(1000000); | |
| minimizer->SetMaxIterations(10000); | |
| minimizer->SetTolerance(0.1); | |
| minimizer->SetPrintLevel(1); | |
| ROOT::Math::Functor f(this,&GlobaldEdxFitter::get_fitquality_functor,1); | |
| double step[1] = {.01}; | |
| double variable[1] = {20.}; | |
| minimizer->SetFunction(f); | |
| minimizer->SetVariable(0,"A",variable[0],step[0]); | |
| minimizer->Minimize(); | |
| const double *xs = minimizer->X(); | |
| return xs[0]; | |
| } | |
| std::unique_ptr<ROOT::Math::Minimizer> minimizer( | |
| ROOT::Math::Factory::CreateMinimizer("Minuit2")); | |
| minimizer->SetMaxFunctionCalls(1000000); | |
| minimizer->SetMaxIterations(10000); | |
| minimizer->SetTolerance(0.1); | |
| minimizer->SetPrintLevel(1); | |
| ROOT::Math::Functor f(this,&GlobaldEdxFitter::get_fitquality_functor,1); | |
| double step[1] = {.01}; | |
| double variable[1] = {20.}; | |
| minimizer->SetFunction(f); | |
| minimizer->SetVariable(0,"A",variable[0],step[0]); | |
| minimizer->Minimize(); | |
| const double *xs = minimizer->X(); | |
| return xs[0]; | |
| } |
Build & test reportReport for commit b209d8585c6ec207b4c60215f8996bb185be86ad:
Automatically generated by sPHENIX Jenkins continuous integration |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
calibrations/tpc/dEdx/dEdxFitter.cc (1)
81-100: Check for missing TPC seed before calling get_dedx.You guard
get_silicon_seed()but still callget_dedx(), which usestrack->get_tpc_seed()unconditionally. If a track has a silicon seed but no TPC seed, this will crash. Add a TPC seed check beforeget_dedx().Proposed fix
if(!track->get_silicon_seed()) { if(Verbosity()>1) { std::cout << "TPC-only track, skipping..." << std::endl; } continue; } + if(!track->get_tpc_seed()) + { + if(Verbosity()>1) + { + std::cout << "No TPC seed, skipping..." << std::endl; + } + continue; + } std::tuple<int,int,int> nclus = get_nclus(track);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
calibrations/tpc/dEdx/GlobaldEdxFitter.cccalibrations/tpc/dEdx/GlobaldEdxFitter.hcalibrations/tpc/dEdx/Makefile.amcalibrations/tpc/dEdx/bethe_bloch.hcalibrations/tpc/dEdx/dEdxFitter.cccalibrations/tpc/dEdx/dEdxFitter.hcalibrations/tpc/dEdx/test_sample_size.C
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cc,cpp,cxx,c}
⚙️ CodeRabbit configuration file
**/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
Flag hidden global state, non-const singletons, and unclear lifetime assumptions.Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.
Files:
calibrations/tpc/dEdx/dEdxFitter.cccalibrations/tpc/dEdx/GlobaldEdxFitter.cc
**/*.{h,hpp,hxx,hh}
⚙️ CodeRabbit configuration file
**/*.{h,hpp,hxx,hh}: Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete.
If interfaces change, ask for compatibility notes and any needed downstream updates.Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.
Files:
calibrations/tpc/dEdx/dEdxFitter.hcalibrations/tpc/dEdx/GlobaldEdxFitter.hcalibrations/tpc/dEdx/bethe_bloch.h
**/*.C
⚙️ CodeRabbit configuration file
Do NOT review these files
Files:
calibrations/tpc/dEdx/test_sample_size.C
🧬 Code graph analysis (4)
calibrations/tpc/dEdx/dEdxFitter.cc (2)
calibrations/tpc/dEdx/dEdxFitter.h (1)
dEdxFitter(19-86)simulation/g4simulation/g4eval/SvtxHitEval.h (1)
_clustermap(84-84)
calibrations/tpc/dEdx/dEdxFitter.h (3)
calibrations/tpc/dEdx/dEdxFitter.cc (7)
dEdxFitter(13-18)InitRun(21-27)InitRun(21-21)process_event(31-50)process_event(31-31)End(205-247)End(205-205)offline/framework/fun4all/SubsysReco.h (1)
SubsysReco(23-74)calibrations/tpc/dEdx/GlobaldEdxFitter.h (1)
GlobaldEdxFitter(14-21)
calibrations/tpc/dEdx/GlobaldEdxFitter.h (1)
calibrations/tpc/dEdx/GlobaldEdxFitter.cc (34)
processResidualData(13-61)processResidualData(13-13)addTrack(63-67)addTrack(63-63)get_fitquality(148-211)get_fitquality(148-148)get_fitquality_new(69-146)get_fitquality_new(69-69)create_TF1(277-281)create_TF1(277-277)create_TF2(283-287)create_TF2(283-283)create_TF3_new(271-275)create_TF3_new(271-271)get_minimum(316-323)get_minimum(316-316)get_minimum_new(289-314)get_minimum_new(289-289)get_minimum_ZS(325-333)get_minimum_ZS(325-325)get_betagamma(213-249)get_betagamma(213-213)graph_vsbetagamma(335-340)graph_vsbetagamma(335-335)graph_vsp(342-346)graph_vsp(342-342)get_fitquality_functor(251-254)get_fitquality_functor(251-251)get_fitquality_wrapper(256-259)get_fitquality_wrapper(256-256)get_fitquality_wrapper_ZS(261-264)get_fitquality_wrapper_ZS(261-261)get_fitquality_wrapper_new(266-269)get_fitquality_wrapper_new(266-266)
calibrations/tpc/dEdx/GlobaldEdxFitter.cc (1)
calibrations/tpc/dEdx/bethe_bloch.h (2)
bethe_bloch_new_1D(48-53)bethe_bloch_total(66-71)
🪛 Clang (14.0.6)
calibrations/tpc/dEdx/dEdxFitter.h
[error] 4-4: 'fun4all/SubsysReco.h' file not found
(clang-diagnostic-error)
calibrations/tpc/dEdx/bethe_bloch.h
[error] 4-4: 'TMath.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (1)
calibrations/tpc/dEdx/bethe_bloch.h (1)
1-208: Previous review concerns addressedThe
inlinekeyword has been added to all function definitions, resolving the ODR violation. TheTMath.hinclude now uses angle brackets. The static analyzer warning aboutTMath.hnot found is a false positive—ROOT headers are resolved via the build system's include paths (typically$(ROOTSYS)/include).Physics constants and Bethe-Bloch formulations look appropriate for sPHENIX TPC gas mixture calibration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
calibrations/tpc/dEdx/dEdxFitter.cc
Outdated
| void dEdxFitter::process_tracks(PHCompositeNode *topNode) | ||
| { | ||
|
|
||
| for(const auto &[key, track] : *_trackmap) | ||
| { |
There was a problem hiding this comment.
Guard against missing node pointers before dereference.
process_tracks() iterates _trackmap and calls helpers that dereference _clustermap, _geometry, _tpcgeom, and _vertexmap even when GetNodes() failed (it only logs). This can crash on runs with missing nodes. Add a fast-fail guard before the loop.
Proposed fix
void dEdxFitter::process_tracks(PHCompositeNode *topNode)
{
+ if(!_trackmap || !_clustermap || !_geometry || !_tpcgeom || !_vertexmap)
+ {
+ if(Verbosity()>0)
+ {
+ std::cout << PHWHERE << " missing required nodes, skipping event" << std::endl;
+ }
+ return;
+ }
for(const auto &[key, track] : *_trackmap)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void dEdxFitter::process_tracks(PHCompositeNode *topNode) | |
| { | |
| for(const auto &[key, track] : *_trackmap) | |
| { | |
| void dEdxFitter::process_tracks(PHCompositeNode *topNode) | |
| { | |
| if(!_trackmap || !_clustermap || !_geometry || !_tpcgeom || !_vertexmap) | |
| { | |
| if(Verbosity()>0) | |
| { | |
| std::cout << PHWHERE << " missing required nodes, skipping event" << std::endl; | |
| } | |
| return; | |
| } | |
| for(const auto &[key, track] : *_trackmap) | |
| { |
| double GlobaldEdxFitter::get_fitquality_wrapper(double* x, double* par) | ||
| { | ||
| return get_fitquality(x[0]); | ||
| } |
There was a problem hiding this comment.
Missing second argument in get_fitquality call
get_fitquality_wrapper passes only x[0] to get_fitquality(double norm, double ZS_loss), which requires two parameters. This will either fail to compile or invoke undefined behavior if there's a declaration mismatch.
Proposed fix
If ZS_loss should default to zero when using this wrapper:
double GlobaldEdxFitter::get_fitquality_wrapper(double* x, double* par)
{
- return get_fitquality(x[0]);
+ return get_fitquality(x[0], 0.0);
}Alternatively, if par should supply ZS_loss:
double GlobaldEdxFitter::get_fitquality_wrapper(double* x, double* par)
{
- return get_fitquality(x[0]);
+ return get_fitquality(x[0], par[0]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| double GlobaldEdxFitter::get_fitquality_wrapper(double* x, double* par) | |
| { | |
| return get_fitquality(x[0]); | |
| } | |
| double GlobaldEdxFitter::get_fitquality_wrapper(double* x, double* par) | |
| { | |
| return get_fitquality(x[0], par[0]); | |
| } |
| ROOT::Math::Minimizer* minimizer = ROOT::Math::Factory::CreateMinimizer("Minuit2"); | ||
| minimizer->SetMaxFunctionCalls(1000000); | ||
| minimizer->SetMaxIterations(10000); | ||
| minimizer->SetTolerance(0.1); | ||
| minimizer->SetPrintLevel(1); | ||
| ROOT::Math::Functor f(this,&GlobaldEdxFitter::get_fitquality_functor,1); | ||
| double step[1] = {.01}; | ||
| double variable[1] = {20.}; | ||
| minimizer->SetFunction(f); | ||
| minimizer->SetVariable(0,"A",variable[0],step[0]); | ||
| minimizer->Minimize(); | ||
| const double *xs = minimizer->X(); | ||
| delete minimizer; | ||
| return xs[0]; | ||
| } |
There was a problem hiding this comment.
Use-after-free: accessing minimizer result after deletion
minimizer->X() returns a pointer to internal storage owned by the minimizer. Deleting the minimizer at line 312 invalidates xs, so reading xs[0] at line 313 is undefined behavior.
Proposed fix
minimizer->Minimize();
const double *xs = minimizer->X();
+ const double result = xs[0];
delete minimizer;
- return xs[0];
+ return result;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ROOT::Math::Minimizer* minimizer = ROOT::Math::Factory::CreateMinimizer("Minuit2"); | |
| minimizer->SetMaxFunctionCalls(1000000); | |
| minimizer->SetMaxIterations(10000); | |
| minimizer->SetTolerance(0.1); | |
| minimizer->SetPrintLevel(1); | |
| ROOT::Math::Functor f(this,&GlobaldEdxFitter::get_fitquality_functor,1); | |
| double step[1] = {.01}; | |
| double variable[1] = {20.}; | |
| minimizer->SetFunction(f); | |
| minimizer->SetVariable(0,"A",variable[0],step[0]); | |
| minimizer->Minimize(); | |
| const double *xs = minimizer->X(); | |
| delete minimizer; | |
| return xs[0]; | |
| } | |
| ROOT::Math::Minimizer* minimizer = ROOT::Math::Factory::CreateMinimizer("Minuit2"); | |
| minimizer->SetMaxFunctionCalls(1000000); | |
| minimizer->SetMaxIterations(10000); | |
| minimizer->SetTolerance(0.1); | |
| minimizer->SetPrintLevel(1); | |
| ROOT::Math::Functor f(this,&GlobaldEdxFitter::get_fitquality_functor,1); | |
| double step[1] = {.01}; | |
| double variable[1] = {20.}; | |
| minimizer->SetFunction(f); | |
| minimizer->SetVariable(0,"A",variable[0],step[0]); | |
| minimizer->Minimize(); | |
| const double *xs = minimizer->X(); | |
| const double result = xs[0]; | |
| delete minimizer; | |
| return result; | |
| } |
Build & test reportReport for commit 8cbb1765262ad38a115da7eb771819a1fb82cc37:
Automatically generated by sPHENIX Jenkins continuous integration |
|
I added this new package to the build list. However, I don't have permission to push to your fork since you forked from Christof's coresoftware instead of the sPHENIX coresoftware, so can you push a commit to re-trigger jenkins now that this package is actually in the build? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
calibrations/tpc/dEdx/dEdxFitter.cc (1)
53-57: Guard against missing nodes before dereferencing.
GetNodes()only logs;process_tracks()immediately dereferences_trackmap,_clustermap,_geometry,_tpcgeom, and_vertexmap. If any are missing, this will crash the job.Proposed fix
void dEdxFitter::process_tracks() { + if(!_trackmap || !_clustermap || !_geometry || !_tpcgeom || !_vertexmap) + { + if(Verbosity()>0) + { + std::cout << PHWHERE << " missing required nodes, skipping event" << std::endl; + } + return; + } for(const auto &[key, track] : *_trackmap) {calibrations/tpc/dEdx/GlobaldEdxFitter.cc (2)
40-53: Prevent out‑of‑rangeGetEntrywhenskipexceeds available entries.The loop only breaks on
entry == total_entries, soskip > total_entriesstill callsGetEntryon invalid entries and can reuse stale data.Proposed fix
size_t total_entries = t->GetEntriesFast(); - for(size_t entry=skip; entry<(skip+ntracks); entry++) + const size_t end_entry = (skip + ntracks < total_entries) ? (skip + ntracks) : total_entries; + for(size_t entry = skip; entry < end_entry; ++entry) { - if(entry==total_entries) - { - break; - } if(entry % 1000 == 0) { std::cout << entry << std::endl; }
300-313: Avoid use‑after‑free of the minimizer result.
minimizer->X()returns internal storage owned by the minimizer; deleting it before copying the value makesxsinvalid.Proposed fix
minimizer->Minimize(); const double *xs = minimizer->X(); + const double bestA = xs[0]; delete minimizer; - return xs[0]; + return bestA;
| #include <fun4all/SubsysReco.h> | ||
| #include <string> | ||
| #include <trackbase_historic/SvtxTrackMap.h> | ||
| #include <trackbase/TrkrClusterContainer.h> | ||
| #include <trackbase/ActsGeometry.h> | ||
| #include <g4detectors/PHG4TpcGeomContainer.h> | ||
| #include <globalvertex/SvtxVertexMap.h> | ||
|
|
||
| #include "GlobaldEdxFitter.h" | ||
|
|
There was a problem hiding this comment.
Add missing STL includes to keep this public header self‑contained.
The header uses std::unique_ptr, std::vector, std::tuple, and size_t but doesn’t include their headers, which can break downstream compile order.
Proposed fix
`#include` <fun4all/SubsysReco.h>
`#include` <string>
+#include <cstddef>
+#include <memory>
+#include <tuple>
+#include <vector>
`#include` <trackbase_historic/SvtxTrackMap.h>
`#include` <trackbase/TrkrClusterContainer.h>
`#include` <trackbase/ActsGeometry.h>
`#include` <g4detectors/PHG4TpcGeomContainer.h>
`#include` <globalvertex/SvtxVertexMap.h>🧰 Tools
🪛 Clang (14.0.6)
[error] 4-4: 'fun4all/SubsysReco.h' file not found
(clang-diagnostic-error)
| #ifndef GLOBALDEDXFITTER_H | ||
| #define GLOBALDEDXFITTER_H | ||
|
|
||
| #include "bethe_bloch.h" | ||
| #include "TF1.h" | ||
| #include "TF2.h" | ||
| #include "TF3.h" | ||
| #include "TChain.h" | ||
| #include "TGraph.h" | ||
| #include "Math/Minimizer.h" | ||
| #include "Math/Functor.h" | ||
| #include "Math/Factory.h" | ||
|
|
There was a problem hiding this comment.
Make GlobaldEdxFitter.h self‑contained by adding STL includes.
This header uses std::string, std::vector, std::pair, and size_t without including their headers, which can fail in downstream TUs.
Proposed fix
`#ifndef` GLOBALDEDXFITTER_H
`#define` GLOBALDEDXFITTER_H
+#include <cstddef>
+#include <string>
+#include <utility>
+#include <vector>
+
`#include` "bethe_bloch.h"
`#include` "TF1.h"
`#include` "TF2.h"
`#include` "TF3.h"
`#include` "TChain.h"
`#include` "TGraph.h"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef GLOBALDEDXFITTER_H | |
| #define GLOBALDEDXFITTER_H | |
| #include "bethe_bloch.h" | |
| #include "TF1.h" | |
| #include "TF2.h" | |
| #include "TF3.h" | |
| #include "TChain.h" | |
| #include "TGraph.h" | |
| #include "Math/Minimizer.h" | |
| #include "Math/Functor.h" | |
| #include "Math/Factory.h" | |
| `#ifndef` GLOBALDEDXFITTER_H | |
| `#define` GLOBALDEDXFITTER_H | |
| `#include` <cstddef> | |
| `#include` <string> | |
| `#include` <utility> | |
| `#include` <vector> | |
| `#include` "bethe_bloch.h" | |
| `#include` "TF1.h" | |
| `#include` "TF2.h" | |
| `#include` "TF3.h" | |
| `#include` "TChain.h" | |
| `#include` "TGraph.h" | |
| `#include` "Math/Minimizer.h" | |
| `#include` "Math/Functor.h" | |
| `#include` "Math/Factory.h" |
Build & test reportReport for commit 0296552dac10e4de6182cf5c8138940d1297436e:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 9a19bb92da1535491aeb611f05c6161932b82d11:
Automatically generated by sPHENIX Jenkins continuous integration |
2 participants
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Adds the dE/dx parametrization to the TPC calibrations folder. The module dEdxFitter produces TF1s for the pion, kaon, proton and deuteron bands using an unbinned-fit procedure that minimizes the chi^2/ndf of the four bands to their closest respective sets of tracks.
The output distributions have a single parameter, the normalization constant (the mean excitation is taken to be the expected value calculated from the sPHENIX TPC gas mixture, and the fit function does not include density-effect contributions), which is extracted from batches of tracks that pass configurable quality cuts. The batch size is also a configurable parameter. When End() is called, the fit results from all batches are averaged to produce the final results. If the run was too small to fill one batch, a fit is performed with whatever population of tracks the module has managed to accumulate.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
This module is currently a duplicate of this module from the analysis repository: https://github.com/sPHENIX-Collaboration/analysis/tree/master/Tracking/dEdx
dEdxFitter Module Addition to TPC Calibrations
Motivation / context
Key changes
Potential risk areas
Possible future improvements
Caveat