Initial version. Requires later updates to remove reference to user …#4288
Conversation
📝 WalkthroughWalkthroughThis PR introduces PHGarfield, a Fun4All module integrating the Garfield electron-transport library. It provides the complete build configuration (Automake/Autoconf), the SubsysReco class with field callbacks and drift-trajectory visualization, and two standalone tools for gas table generation and validation. ChangesPHGarfield integration
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)offline/packages/PHGarfield/MergeGasFiles.ccoffline/packages/PHGarfield/MergeGasFiles.cc:1:10: fatal error: 'TMath.h' file not found ... [truncated 1091 characters] ... include" offline/packages/PHGarfield/GasModel.ccoffline/packages/PHGarfield/GasModel.cc:4:10: fatal error: 'Garfield/ComponentUser.hh' file not found ... [truncated 1126 characters] ... clang/18/include" offline/packages/PHGarfield/PHGarfield.ccIn file included from offline/packages/PHGarfield/PHGarfield.cc:1: ... [truncated 1187 characters] ... 18/include" 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: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e0f17e5-1657-436d-af8c-fca4e98300c8
📒 Files selected for processing (11)
offline/packages/PHGarfield/GasModel.ccoffline/packages/PHGarfield/Makefile.amoffline/packages/PHGarfield/MergeGasFiles.ccoffline/packages/PHGarfield/PHGarfield.ccoffline/packages/PHGarfield/PHGarfield.hoffline/packages/PHGarfield/PHGarfieldLinkDef.hoffline/packages/PHGarfield/autogen.shoffline/packages/PHGarfield/configure.acoffline/packages/PHGarfield/macros/GasModel_condor.joboffline/packages/PHGarfield/macros/StartScript.shoffline/packages/PHGarfield/macros/TestFieldMap.C
| (cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share;\ | ||
| libtoolize --force; automake -a --add-missing; autoconf) | ||
|
|
||
| $srcdir/configure "$@" |
There was a problem hiding this comment.
Fail fast during autotools bootstrap instead of masking intermediate failures.
Using ; means a failing aclocal/libtoolize/automake can be hidden if a later command succeeds, and the script still proceeds to configure.
Proposed fix
#!/bin/sh
-srcdir=`dirname $0`
+set -e
+srcdir="$(dirname "$0")"
test -z "$srcdir" && srcdir=.
-(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
+)
-$srcdir/configure "$@"
+"$srcdir/configure" "$@"🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 5-5: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 5-5: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 8-8: Double quote to prevent globbing and word splitting.
(SC2086)
| const double Emin = std::atof(argv[1]); | ||
| const double Emax = std::atof(argv[2]); | ||
| const int nE = std::atoi(argv[3]); | ||
|
|
||
| const double Bmin = std::atof(argv[4]); | ||
| const double Bmax = std::atof(argv[5]); | ||
| const int nB = std::atoi(argv[6]); | ||
|
|
||
| const double Amin = std::atof(argv[7]); | ||
| const double Amax = std::atof(argv[8]); | ||
| const int nA = std::atoi(argv[9]); | ||
|
|
There was a problem hiding this comment.
Validate grid bounds and bin counts before calling SetFieldGrid.
The program accepts nE/nB/nA and min/max ranges without sanity checks. Zero/negative counts or inverted ranges can produce invalid gas-table requests and runtime failures.
Proposed fix
const double Amin = std::atof(argv[7]);
const double Amax = std::atof(argv[8]);
const int nA = std::atoi(argv[9]);
+ if (nE <= 0 || nB <= 0 || nA <= 0) {
+ std::cerr << "Grid sizes must be > 0.\n";
+ return 1;
+ }
+ if (Emin > Emax || Bmin > Bmax || Amin > Amax) {
+ std::cerr << "Each min value must be <= max value.\n";
+ return 1;
+ }
+
const string output_file(argv[10]);As per coding guidelines: **/*.{cc,cpp,cxx,c} reviews must prioritize correctness/error handling and only raise Critical or Major findings.
Also applies to: 74-76
| output="/direct/phenix+u/workarea/hemmick/code.sphenix/tkh/gas/gasfiles/PART_"$1".gas" | ||
|
|
||
| echo $output | ||
|
|
||
| echo GasModel $emin $emax $ne $bnow $bnow 1 $amin $amax $na $output | ||
| GasModel $emin $emax $ne $bnow $bnow 1 $amin $amax $na $output | ||
|
|
||
| echo all done |
There was a problem hiding this comment.
Preserve command failure status and avoid hardcoded output path.
The script currently masks GasModel failures (final echo returns 0) and writes to a user-specific absolute path, which breaks portability and can mark failed jobs as successful.
Proposed fix
#! /bin/bash
+set -euo pipefail
@@
-output="/direct/phenix+u/workarea/hemmick/code.sphenix/tkh/gas/gasfiles/PART_"$1".gas"
+script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+mkdir -p "${script_dir}/gasfiles"
+output="${script_dir}/gasfiles/PART_${1}.gas"
@@
-GasModel $emin $emax $ne $bnow $bnow 1 $amin $amax $na $output
+GasModel "$emin" "$emax" "$ne" "$bnow" "$bnow" 1 "$amin" "$amax" "$na" "$output"
echo all done📝 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.
| output="/direct/phenix+u/workarea/hemmick/code.sphenix/tkh/gas/gasfiles/PART_"$1".gas" | |
| echo $output | |
| echo GasModel $emin $emax $ne $bnow $bnow 1 $amin $amax $na $output | |
| GasModel $emin $emax $ne $bnow $bnow 1 $amin $amax $na $output | |
| echo all done | |
| script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| mkdir -p "${script_dir}/gasfiles" | |
| output="${script_dir}/gasfiles/PART_${1}.gas" | |
| echo $output | |
| echo GasModel "$emin" "$emax" "$ne" "$bnow" "$bnow" 1 "$amin" "$amax" "$na" "$output" | |
| GasModel "$emin" "$emax" "$ne" "$bnow" "$bnow" 1 "$amin" "$amax" "$na" "$output" | |
| echo all done |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 24-24: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[info] 26-26: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 28-28: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 28-28: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 28-28: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 29-29: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 29-29: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 29-29: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 31-31: Use semicolon or linefeed before 'done' (or quote to make it literal).
(SC1010)
| double DelVx = (vx-vx0)/((vx+vx0)/2.); | ||
| double DelVy = (vy-vy0)/((vy+vy0)/2.); | ||
| double DelVz = (vz-vz0)/((vz+vz0)/2.); | ||
| Validity->Fill(1,sqrt(ex*ex + ey*ey + ez*ez),sqrt(bx*bx + by*by + bz*bz),a,DelVx, DelVy, DelVz); |
There was a problem hiding this comment.
Guard the relative-difference denominator to avoid NaN/Inf in validation output.
If (v + v0) is zero (or very small), these divisions produce unstable/invalid values that pollute GarfieldValidity.root.
Proposed fix
+ auto rel_diff = [](double v, double v0) {
+ const double denom = 0.5 * (v + v0);
+ constexpr double eps = 1e-12;
+ if (std::abs(denom) < eps) return 0.0;
+ return (v - v0) / denom;
+ };
...
- double DelVx = (vx-vx0)/((vx+vx0)/2.);
- double DelVy = (vy-vy0)/((vy+vy0)/2.);
- double DelVz = (vz-vz0)/((vz+vz0)/2.);
+ double DelVx = rel_diff(vx, vx0);
+ double DelVy = rel_diff(vy, vy0);
+ double DelVz = rel_diff(vz, vz0);
...
- double DelVx = (vx-vx0)/((vx+vx0)/2.);
- double DelVy = (vy-vy0)/((vy+vy0)/2.);
- double DelVz = (vz-vz0)/((vz+vz0)/2.);
+ double DelVx = rel_diff(vx, vx0);
+ double DelVy = rel_diff(vy, vy0);
+ double DelVz = rel_diff(vz, vz0);As per coding guidelines: **/*.{cc,cpp,cxx,c} reviews must prioritize correctness/error handling and only raise Critical or Major findings.
Also applies to: 211-214
| m_component = new Garfield::ComponentUser(); | ||
| m_component->SetMagneticField([this](double x, double y, double z,double& bx, double& by, double& bz) { GetMagneticFieldTesla(x, y, z, bx, by, bz); }); | ||
| m_component->SetElectricField([this](double x, double y, double z,double& ex, double& ey, double& ez) { GetElectricFieldVcm (x, y, z, ex, ey, ez); }); | ||
| InitializeGas("/direct/phenix+u/workarea/hemmick/code.sphenix/tkh/gas/gasfiles/"); |
There was a problem hiding this comment.
Fail InitRun when gas table loading fails, and remove user-specific path coupling.
Line 81 hardcodes a user directory, and InitializeGas (Lines 221-262) only logs on failure but does not propagate status. Line 87 still returns EVENT_OK, so the subsystem continues after failed gas setup.
Suggested fix pattern
- InitializeGas("/direct/phenix+u/workarea/hemmick/code.sphenix/tkh/gas/gasfiles/");
+ const std::string gas_dir = /* resolve from config/CDB/env */;
+ if (!InitializeGas(gas_dir))
+ {
+ std::cerr << "PHGarfield: gas initialization failed for " << gas_dir << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
...
-void PHGarfield::InitializeGas(std::string dir)
+bool PHGarfield::InitializeGas(const std::string& dir)
{
- m_gas = new Garfield::MediumMagboltz();
+ m_gas = new Garfield::MediumMagboltz();
...
- return;
+ return false;
...
- return;
+ return false;
...
- return;
+ return false;
...
+ return true;
}As per coding guidelines **/*.{cc,cpp,cxx,c}: “Prioritize correctness, memory safety, error handling, and thread-safety.”
Also applies to: 221-262, 87-87
| TPolyLine3D *PHGarfield::ReverseDrift (double x, double y, double z, double step_ns) | ||
| { | ||
| vector<double> xlist; | ||
| vector<double> ylist; | ||
| vector<double> zlist; | ||
|
|
||
| xlist.push_back(x); | ||
| ylist.push_back(y); | ||
| zlist.push_back(z); | ||
|
|
||
| double ex, ey, ez, bx, by, bz, vx, vy, vz; | ||
|
|
||
| double zPrevious = z; | ||
| while (!StopHere(x,y,z,zPrevious)) | ||
| { | ||
| zPrevious = z; | ||
| GetMagneticFieldTesla( x, y, z, bx, by, bz); | ||
| GetElectricFieldVcm ( x, y, z, ex, ey, ez); | ||
| m_gas->ElectronVelocity(ex, ey, ez, bx, by, bz, vx, vy, vz); | ||
|
|
||
| x = x - vx*step_ns; | ||
| y = y - vy*step_ns; | ||
| z = z - vz*step_ns; | ||
|
|
||
| xlist.push_back(x); | ||
| ylist.push_back(y); | ||
| zlist.push_back(z); | ||
| } |
There was a problem hiding this comment.
Guard ReverseDrift against non-positive step_ns to avoid hangs.
If step_ns <= 0, the state does not progress toward termination and the loop at Line 299 can become non-terminating.
Minimal guard
TPolyLine3D *PHGarfield::ReverseDrift (double x, double y, double z, double step_ns)
{
+ if (step_ns <= 0.0)
+ {
+ std::cerr << "ReverseDrift requires step_ns > 0" << std::endl;
+ return new TPolyLine3D(0);
+ }
vector<double> xlist;As per coding guidelines **/*.{cc,cpp,cxx,c}: “Prioritize correctness … and thread-safety.”
📝 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.
| TPolyLine3D *PHGarfield::ReverseDrift (double x, double y, double z, double step_ns) | |
| { | |
| vector<double> xlist; | |
| vector<double> ylist; | |
| vector<double> zlist; | |
| xlist.push_back(x); | |
| ylist.push_back(y); | |
| zlist.push_back(z); | |
| double ex, ey, ez, bx, by, bz, vx, vy, vz; | |
| double zPrevious = z; | |
| while (!StopHere(x,y,z,zPrevious)) | |
| { | |
| zPrevious = z; | |
| GetMagneticFieldTesla( x, y, z, bx, by, bz); | |
| GetElectricFieldVcm ( x, y, z, ex, ey, ez); | |
| m_gas->ElectronVelocity(ex, ey, ez, bx, by, bz, vx, vy, vz); | |
| x = x - vx*step_ns; | |
| y = y - vy*step_ns; | |
| z = z - vz*step_ns; | |
| xlist.push_back(x); | |
| ylist.push_back(y); | |
| zlist.push_back(z); | |
| } | |
| TPolyLine3D *PHGarfield::ReverseDrift (double x, double y, double z, double step_ns) | |
| { | |
| if (step_ns <= 0.0) | |
| { | |
| std::cerr << "ReverseDrift requires step_ns > 0" << std::endl; | |
| return new TPolyLine3D(0); | |
| } | |
| vector<double> xlist; | |
| vector<double> ylist; | |
| vector<double> zlist; | |
| xlist.push_back(x); | |
| ylist.push_back(y); | |
| zlist.push_back(z); | |
| double ex, ey, ez, bx, by, bz, vx, vy, vz; | |
| double zPrevious = z; | |
| while (!StopHere(x,y,z,zPrevious)) | |
| { | |
| zPrevious = z; | |
| GetMagneticFieldTesla( x, y, z, bx, by, bz); | |
| GetElectricFieldVcm ( x, y, z, ex, ey, ez); | |
| m_gas->ElectronVelocity(ex, ey, ez, bx, by, bz, vx, vy, vz); | |
| x = x - vx*step_ns; | |
| y = y - vy*step_ns; | |
| z = z - vz*step_ns; | |
| xlist.push_back(x); | |
| ylist.push_back(y); | |
| zlist.push_back(z); | |
| } |
| { | ||
| public: | ||
| PHGarfield(const std::string &name = "PHGarfield"); | ||
| ~PHGarfield() override = default; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the header
ls -la offline/packages/PHGarfield/PHGarfield.h 2>/dev/null || true
# Show the relevant parts of the header (around destructor + members)
sed -n '1,120p' offline/packages/PHGarfield/PHGarfield.h | nl -ba
# Find InitRun implementation and inspect allocations
rg -n "InitRun\\s*\\(" -S . || true
rg -n "PHGarfield::InitRun" -S . || true
# If an implementation file exists, print it around the allocation sites.
# Best-effort: search for the member names in InitRun or around allocations.
rg -n "m_field|m_cdbTPCMAPttree|m_component|m_gas" offline/packages/PHGarfield -S || true
# Also check if there is any destructor / cleanup elsewhere for these members.
rg -n "~PHGarfield|Cleanup|Finalize|clear\\(|delete\\s+(m_field|m_cdbTPCMAPttree|m_component|m_gas)" -S offline/packages/PHGarfield || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
H="offline/packages/PHGarfield/PHGarfield.h"
ls -la "$H" 2>/dev/null || true
echo "=== PHGarfield.h (first 220 lines) ==="
sed -n '1,220p' "$H" || true
echo "=== PHGarfield.h: members + destructor area ==="
rg -n "class\\s+PHGarfield|~PHGarfield|InitRun|m_field|m_cdbTPCMAPttree|m_component|m_gas" "$H" -n -S || true
echo "=== Find InitRun in PHGarfield package ==="
rg -n "InitRun\\s*\\(" -S offline/packages/PHGarfield || true
rg -n "PHGarfield::InitRun" -S offline/packages/PHGarfield || true
echo "=== Locate member cleanup / deletes in PHGarfield package ==="
rg -n "~PHGarfield|Cleanup|Finalize|clear\\(|delete\\s+(m_field|m_cdbTPCMAPttree|m_component|m_gas)" -S offline/packages/PHGarfield || true
echo "=== Show PHGarfield*.cc/.cpp/.cxx/.hxx for InitRun allocations context ==="
ls -la offline/packages/PHGarfield || true
fd -t f -ecc -ecc -e cpp -e cxx -e cc -e c -e h -e hxx -e hh offline/packages/PHGarfield -x echo >/dev/null 2>&1 || true
fd -t f offline/packages/PHGarfield -g '*PHGarfield*' || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 4829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
H="offline/packages/PHGarfield/PHGarfield.cc"
echo "=== PHGarfield.cc size (lines) ==="
wc -l "$H" || true
echo "=== PHGarfield.cc: include + member allocations context near InitRun ==="
sed -n '1,120p' "$H" || true
echo "=== PHGarfield.cc: InitRun body (line-focused) ==="
# Print a window around InitRun definition
LINE=$(rg -n "int PHGarfield::InitRun" "$H" | head -n1 | cut -d: -f1)
echo "InitRun line: $LINE"
START=$((LINE-10))
END=$((LINE+220))
sed -n "${START},${END}p" "$H" || true
echo "=== PHGarfield.cc: occurrences of member pointers ==="
rg -n "m_field|m_cdbTPCMAPttree|m_component|m_gas" "$H" -n || true
echo "=== PHGarfield.cc: delete/free calls ==="
rg -n "delete\\s+(m_field|m_cdbTPCMAPttree|m_component|m_gas)|delete\\s+.*m_field|delete\\s+.*m_cdbTPCMAPttree|delete\\s+.*m_component|delete\\s+.*m_gas|free\\(|Destroy|Finalize|Stop\\b|End\\b" "$H" -S || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 12040
Fix ownership of PHGarfield raw pointers to prevent leaks (RAII)
PHGarfield::~PHGarfield() is defaulted inline, while PHGarfield::InitRun allocates m_field (new PHField3DCartesian), m_cdbTPCMAPttree (new CDBTTree), m_component (new Garfield::ComponentUser), and m_gas (new Garfield::MediumMagboltz) with no corresponding delete/cleanup in PHGarfield.cc, so memory leaks on teardown and any InitRun re-initialization path.
Switch these owning members to std::unique_ptr and make sure the destructor is defined out-of-line (or include complete type headers) so unique_ptr can safely delete the forward-declared types.
Build & test reportReport for commit 708f7774f9da38341ea24861dfd4a552e9a52eb8:
Automatically generated by sPHENIX Jenkins continuous integration |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/PHGarfield/PHGarfield.cc (1)
101-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential out-of-bounds write if CDB returns unexpected layer values.
radiiis declared asdouble radii[48]{}in the header. The indexingradii[layer - 7]assumes7 ≤ layer ≤ 54. If the CDB map ever contains a layer value ≥ 55 (e.g., due to data corruption or schema evolution), this writes past the array bounds, causing undefined behavior.Consider adding a bounds check:
Suggested guard
if (layer > 6) { + if (layer - 7 >= 48) + { + std::cerr << "PHGarfield::FillRadii: layer " << layer << " out of range" << std::endl; + continue; + } radii[layer - 7] = r; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 683aa11d-73c1-4311-ab2e-11b8830923c9
📒 Files selected for processing (4)
offline/packages/PHGarfield/GasModel.ccoffline/packages/PHGarfield/MergeGasFiles.ccoffline/packages/PHGarfield/PHGarfield.ccoffline/packages/PHGarfield/PHGarfield.h
🚧 Files skipped from review as they are similar to previous changes (3)
- offline/packages/PHGarfield/PHGarfield.h
- offline/packages/PHGarfield/GasModel.cc
- offline/packages/PHGarfield/MergeGasFiles.cc
Build & test reportReport for commit b2b274d3bc53a742b1dadea98191702448aeb200:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 391555cfaf7c57ff55bb2425370a5e43ad072f9b:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 3f7025659ac13cb1162e67635483be3a2af3e0e5:
Automatically generated by sPHENIX Jenkins continuous integration |
2 participants
…directories.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
PHGarfield Integration – Version 0.9
Motivation & Context
This PR introduces PHGarfield, a Garfield++ integration module for the sPHENIX TPC. The package provides electron drift and field simulation capabilities within the Fun4All framework, enabling detailed tracking and reconstruction studies. This is the initial 0.9 release presented to the Tracking Group on June 4, 2026.
Key Changes
PHGarfield module (
PHGarfield.h/cc): MainSubsysRecoclass integrating Garfield++ with Fun4All. Features include:PART_0.gas,PART_1.gas, etc.)ReverseDrift)StopHere) for radial/z limits and membrane crossingsGasModel utility (
GasModel.cc): Standalone executable to generate gas tables for specified E/B/angle ranges with configurable grid parametersMergeGasFiles utility (
MergeGasFiles.cc): Standalone tool to merge gas file partitions and validate transport property changes via Monte Carlo sampling (outputsGarfieldValidity.root)Build system (
Makefile.am,configure.ac,autogen.sh,PHGarfieldLinkDef.h): Complete autotools configuration for library/executable compilation with ROOT dictionary generationPotential Risk Areas
PHGarfield.cccontains absolute path/direct/phenix+u/workarea/hemmick/code.sphenix/tkh/gas/gasfiles/that must be updated to a portable/configurable locationPART_*.gasfiles exist in a specific directory; missing files silently terminate gas initializationPossible Future Improvements
process_event()(currently a no-op)Note on Summary Accuracy: AI-generated analyses may contain errors or miss important implementation details. Review actual source files for authoritative behavior, especially regarding hardcoded paths, gas mixture specifications, and integration points with CDB and field systems.