PHGarfield updates for variable electric field and CDB interface.#4321
Conversation
📝 WalkthroughWalkthroughThis PR replaces the gas-merge validation utility with a command-line merger for ChangesGas merge tooling and loader updates
Sequence Diagram(s)MergeGasFiles CLI merge flowsequenceDiagram
participant main as main(argc, argv)
participant scan as searchAndUnpackDirectory
participant fs as std::filesystem
participant gas as Garfield::MediumMagboltz
main->>scan: validate path_to_gasfiles
scan->>fs: iterate directory entries
scan-->>main: Eindices, Bindices, FileList
main->>gas: load and merge E/B files
main->>fs: write MERGED_E###.gas and output file
PHGarfield gas loading flowsequenceDiagram
participant init as PHGarfield::InitRun
participant cdb as CDB
participant fs as std::filesystem
participant load as PHGarfield::InitializeGas
participant gas as Garfield::MediumMagboltz
participant summary as PHGarfield::PrintGasSummary
init->>cdb: read PHGARFIELD_GAS
init->>fs: check configured path
init->>load: pass file or directory path
load->>gas: load a file or merge MERGED_E###.gas
load->>summary: print the field grid
summary->>gas: GetFieldGrid(...)
Possibly related PRs
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65662fc1-cb67-454c-b298-51b96c860d77
📒 Files selected for processing (3)
offline/packages/PHGarfield/MergeGasFiles.ccoffline/packages/PHGarfield/PHGarfield.ccoffline/packages/PHGarfield/PHGarfield.h
| if (firstB) | ||
| { | ||
| gas.LoadGasFile(nextfile); | ||
| firstB = false; | ||
| } | ||
| else | ||
| { | ||
| gas.MergeGasFile(nextfile, true); | ||
| } | ||
| } | ||
|
|
||
| const std::string mergedFile = mergedName(path, Eindex); | ||
|
|
||
| std::cout << "Writing " << mergedFile << std::endl; | ||
| gas.WriteGasFile(mergedFile); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check: inspect local Garfield declarations/usages if available.
fd -i 'Medium.*' . --exec rg -n -C2 '\b(LoadGasFile|MergeGasFile|WriteGasFile)\s*\(' {}
rg -n -C2 '\b(LoadGasFile|MergeGasFile|WriteGasFile)\s*\(' offline/packages/PHGarfieldRepository: sPHENIX-Collaboration/coresoftware
Length of output: 3524
🏁 Script executed:
#!/bin/bash
# Find the header file containing the Medium class or Garfield gas file interface
fd -i 'Medium*.h' . --exec rg -n -C5 '\b(LoadGasFile|MergeGasFile)\s*\(' {}
# Also try to find the class definition directly
rg -n '\bclass\s+\w+.*\{[^}]*LoadGasFile' -t hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Search for method definitions (implementation)
rg -n 'bool.*LoadGasFile|bool.*MergeGasFile' -t cc -t h -t C C++
rg -n 'auto.*LoadGasFile|auto.*MergeGasFile' -t cc -t h -t C C++
# Search for includes that might point to external Garfield headers
rg -n '`#include`.*Garfield|Garfield' offline/packages/PHGarfield --include '*.h' --include '*.cc' | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Search for method definitions (implementation) using correct rg syntax
rg -n 'bool.*LoadGasFile|bool.*MergeGasFile' -t c -t cpp
rg -n 'auto.*LoadGasFile|auto.*MergeGasFile' -t c -t cpp
# Search for includes that might point to external Garfield headers
rg -n '`#include`.*Garfield' offline/packages/PHGarfield -A 2
# Check the actual class name used for the gas object in MergeGasFiles.cc
head -100 offline/packages/PHGarfield/MergeGasFiles.cc | rg -A 5 -B 5 'MergeGasFiles'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Search for method definitions using broad pattern matching
rg -n '\bLoadGasFile' offline/packages/PHGarfield -A 5 -B 5 | head -50
# Get the first 50 lines to understand the class structure
head -50 offline/packages/PHGarfield/MergeGasFiles.cc
# Search for type declarations of 'gas' variable
rg -n '^\s*(.*\s+)?gas\s+' offline/packages/PHGarfield/MergeGasFiles.cc | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4343
Fail the merge when Garfield load/merge operations fail.
The LoadGasFile and MergeGasFile methods on Garfield::MediumMagboltz return a boolean status (as established in PHGarfield.cc:258), but MergeGasFiles.cc ignores these values. A corrupt or incompatible gas file can silently produce partial or invalid output (MERGED_E###.gas or final merged file) while the tool reports success.
- Lines 68–76:
gas.LoadGasFile(nextfile)andgas.MergeGasFile(nextfile, true)are called without checking return status. - Lines 109–117: Same issue occurs during the final merge loop.
- Bonus finding:
PHGarfield.cc:295also ignores theLoadGasFilereturn value in its merge loop.
Apply the check used in PHGarfield.cc:258 to all calls in this file:
- gas.LoadGasFile(nextfile);
+ if (!gas.LoadGasFile(nextfile))
+ {
+ std::cerr << PHWHERE << " Failed to load " << nextfile << std::endl;
+ return 1;
+ }
- gas.MergeGasFile(nextfile, true);
+ if (!gas.MergeGasFile(nextfile, true))
+ {
+ std::cerr << PHWHERE << " Failed to merge " << nextfile << std::endl;
+ return 1;
+ }This ensures data integrity by preventing silent corruption of output files.
📝 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.
| if (firstB) | |
| { | |
| gas.LoadGasFile(nextfile); | |
| firstB = false; | |
| } | |
| else | |
| { | |
| gas.MergeGasFile(nextfile, true); | |
| } | |
| } | |
| const std::string mergedFile = mergedName(path, Eindex); | |
| std::cout << "Writing " << mergedFile << std::endl; | |
| gas.WriteGasFile(mergedFile); | |
| if (firstB) | |
| { | |
| if (!gas.LoadGasFile(nextfile)) | |
| { | |
| std::cerr << PHWHERE << " Failed to load " << nextfile << std::endl; | |
| return 1; | |
| } | |
| firstB = false; | |
| } | |
| else | |
| { | |
| if (!gas.MergeGasFile(nextfile, true)) | |
| { | |
| std::cerr << PHWHERE << " Failed to merge " << nextfile << std::endl; | |
| return 1; | |
| } | |
| } | |
| } | |
| const std::string mergedFile = mergedName(path, Eindex); | |
| std::cout << "Writing " << mergedFile << std::endl; | |
| gas.WriteGasFile(mergedFile); |
|
|
||
| // Here we fetch the gas from the CDB | ||
| std::string gasfile = m_cdb->getUrl("PHGARFIELD_GAS"); | ||
| if (gasfile.empty() || !fs::exists(gasfile)) | ||
| { | ||
| std::cerr << PHWHERE << " Missing CDB gasfile: " << gasfile << std::endl; | ||
| std::cerr << PHWHERE << " Using default gasfile: " << m_defaultGasfile << std::endl; | ||
| gasfile = m_defaultGasfile; | ||
| } | ||
| InitializeGas(gasfile); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification of Garfield and Fun4All APIs.
rg -n -C3 '\bMergeGasFile\s*\(' .
rg -n -C3 '\bABORTRUN\b|\bABORTEVENT\b|\bDISCARDEVENT\b' offline/framework offline/packages | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 6425
🌐 Web query:
Garfield++ MediumMagboltz MergeGasFile return type signature
💡 Result:
The return type signature for the MergeGasFile method in the Garfield++ MediumMagboltz class (inherited from MediumGas) is bool [1][2]. The full method signature is: bool MergeGasFile(const std::string &filename, const bool replaceOld) [1][3] This method is used to read a table of gas properties from a specified file and merge it with the existing dataset [1][4]. The replaceOld parameter is a boolean flag indicating whether to replace existing data if conflicts occur [4].
Citations:
- 1: https://garfieldpp.docs.cern.ch/doxygen/classGarfield_1_1MediumMagboltz.html
- 2: https://garfieldpp.docs.cern.ch/doxygen/classGarfield_1_1MediumGas.html
- 3: https://garfieldpp.docs.cern.ch/doxygen/MediumGas_8hh_source.html
- 4: https://garfieldpp.web.cern.ch/doxygen/UserGuide.pdf
🌐 Web query:
Garfield++ MediumMagboltz LoadGasFile return type signature
💡 Result:
In Garfield++, the LoadGasFile method, which is inherited by the MediumMagboltz class from the base class MediumGas [1][2], has the following function signature: bool LoadGasFile(const std::string& filename, const bool quiet = false); This method reads a table of gas properties (transport parameters) from a specified file [1][2]. It returns a boolean value, where true typically indicates success and false indicates an error occurred during the loading process [1][2][3]. The second parameter, quiet, is optional and defaults to false; setting it to true suppresses output messages [2][4].
Citations:
- 1: https://garfieldpp.docs.cern.ch/doxygen/classGarfield_1_1MediumMagboltz.html
- 2: https://garfieldpp.docs.cern.ch/doxygen/classGarfield_1_1MediumGas.html
- 3: https://garfieldpp.web.cern.ch/doxygen/UserGuide.pdf
- 4: https://garfieldpp.docs.cern.ch/doxygen/MediumGas_8hh_source.html
Make gas initialization fail closed instead of continuing with an unloaded gas.
InitializeGas currently returns void and only logs errors, allowing InitRun to proceed with a potentially invalid Garfield::MediumMagboltz object if the gas file or directory loading fails. Garfield++'s LoadGasFile and MergeGasFile both return bool to indicate success. Ignoring these return values risks downstream segmentation faults or incorrect physics results when the gas object is in an uninitialized state.
Adopt a fail-closed pattern by returning Fun4AllReturnCodes::ABORTRUN on any gas initialization failure:
- Change
InitializeGasreturn type tobooland propagate failures fromLoadGasFileandMergeGasFile. - Return
ABORTRUNinInitRunifInitializeGasreturnsfalse. - Verify directory scans yield at least one file before attempting loads.
Suggested changes
PHGarfield.h
- void InitializeGas(const std::string &name);
+ bool InitializeGas(const std::string &name);PHGarfield.cc
- InitializeGas(gasfile);
+ if (!InitializeGas(gasfile))
+ {
+ return Fun4AllReturnCodes::ABORTRUN;
+ }-void PHGarfield::InitializeGas(const std::string &name)
+bool PHGarfield::InitializeGas(const std::string &name)
{
+ m_GasFilesLoaded = false;
// Create and fill the gas object so that we can trace particles through the gas...
m_gas = new Garfield::MediumMagboltz();
if (!std::filesystem::exists(name))
{
std::cerr << "Missing gas file or gas directory: " << name << std::endl;
- return;
+ return false;
}
...
if (!m_gas->LoadGasFile(name))
- {
- std::cerr << "Failed to load " << name << std::endl;
- return;
- }
+ {
+ std::cerr << "Failed to load " << name << std::endl;
+ return false;
+ }
m_GasFilesLoaded = true;
...
if (FilesToMerge.empty())
- {
- std::cerr << PHWHERE << " No MERGED_E###.gas files found in " << name << std::endl;
- }
+ {
+ std::cerr << PHWHERE << " No MERGED_E###.gas files found in " << name << std::endl;
+ return false;
+ }
bool firstE = true;
for (const auto& [key, filepath] : FilesToMerge)
{
if (firstE)
{
- m_gas->LoadGasFile(filepath);
+ if (!m_gas->LoadGasFile(filepath)) { return false; }
firstE = false;
m_GasFilesLoaded = true;
}
else
{
- m_gas->MergeGasFile(filepath, true);
+ if (!m_gas->MergeGasFile(filepath, true)) { return false; }
m_GasFilesLoaded = true;
}
}
}
PrintGasSummary();
+ return true;
}
Build & test reportReport for commit 7f164080b94e5c7cb905963b2aeafedd4e8f8247:
Automatically generated by sPHENIX Jenkins continuous integration |
2 participants
comment: Url gas file lookup and variable E-field.> (Please tell us something about this pull request)
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)
AI-generated summaries can make mistakes, so please use judgment when reviewing this.
Motivation / context
Key changes
.gasfiles discovered in a directory.(E, B)file grid is complete before merging.PHGarfieldto load either a single gas file or a directory of merged gas files.Potential risk areas
Possible future improvements