Conversation
…on; Survey Geometry is on by default
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughChangesTPC field and condition integration
Sequence Diagram(s)sequenceDiagram
participant CDB
participant TpcConditionsReco
participant Tpc_PolyClusterizer
participant PHGarfield
CDB->>TpcConditionsReco: Load TPC conditions
TpcConditionsReco->>Tpc_PolyClusterizer: Provide BCO-dependent loads
Tpc_PolyClusterizer->>PHGarfield: Configure maps, kEff, geometry, and voltages
PHGarfield-->>Tpc_PolyClusterizer: Initialize Garfield field configuration
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Manual and run-dependent reconstruction can use incorrect calibration settings, while existing PHGarfield users may see changed defaults or initialization failures. Resolve these compatibility and configuration issues before merging. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Build & test reportReport for commit f0d8c9cefaa22ccdd6203c1956d0889cccbd1b46:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
This passes the QA and similarly to your slides from today, the jenkins QA shows no change for 79513. I think if you are ready for this to go in we should merge it so that it is in the next ana build
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: sPHENIX-Collaboration/coresoftware/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 99cdff8c-25e4-435c-a735-bfa666e8d8ed
📒 Files selected for processing (10)
offline/framework/phool/RunnumberRange.hoffline/packages/PHGarfield/Makefile.amoffline/packages/PHGarfield/PHGarfield.ccoffline/packages/PHGarfield/PHGarfield.hoffline/packages/TpcConditions/TpcConditions.hoffline/packages/TpcConditions/TpcConditionsReco.ccoffline/packages/TpcConditions/TpcConditionsReco.hoffline/packages/tpctrackreco/Makefile.amoffline/packages/tpctrackreco/Tpc_PolyClusterizer.ccoffline/packages/tpctrackreco/Tpc_PolyClusterizer.h
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| double spaceChargeScale_side0 = std::numeric_limits<double>::quiet_NaN(), | ||
| double spaceChargeScale_side1 = std::numeric_limits<double>::quiet_NaN()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Find all PHGarfield constructions and scale/voltage setter calls outside the clusterizer.
rg -nP -C3 '\bPHGarfield\s*\(' --type=cpp -g '!offline/packages/PHGarfield/**'
rg -nP -C2 'SetFrameChargeScale|SetCMVoltageDefault|SetSpaceChargeScale' --type=cppRepository: sPHENIX-Collaboration/coresoftware
Length of output: 7830
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- header constructor and defaults ---'
sed -n '20,45p;315,360p' offline/packages/PHGarfield/PHGarfield.h
printf '%s\n' '--- implementation constructor and CDB/init paths ---'
rg -n -C5 'PHGarfield::PHGarfield|LoadCDBInputs|ConfigureRunDependentFieldCage|spaceChargeScaleOverride|m_CMVoltageDefault|m_frameChargeScale_side' offline/packages/PHGarfield/PHGarfield.cc
printf '%s\n' '--- all repository PHGarfield references ---'
rg -n -P '\bPHGarfield\b|\bPHGarfield\s*\(' --glob '!offline/packages/PHGarfield/PHGarfield.h' --glob '!offline/packages/PHGarfield/PHGarfield.cc' .Repository: sPHENIX-Collaboration/coresoftware
Length of output: 23662
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- TpcCrossingFinder configuration ---'
sed -n '130,155p;220,245p' offline/packages/tpctrackreco/TpcCrossingFinder.cc
printf '%s\n' '--- Tpc_PolyClusterizer construction and configuration ---'
sed -n '330,445p;455,485p;515,575p' offline/packages/tpctrackreco/Tpc_PolyClusterizer.cc
printf '%s\n' '--- LaserClusterHelper construction and configuration ---'
sed -n '55,85p' offline/packages/tpc/LaserClusterHelper.cc
printf '%s\n' '--- focused PHGarfield diff ---'
git diff -- offline/packages/PHGarfield/PHGarfield.h offline/packages/PHGarfield/PHGarfield.cc | sed -n '1,260p'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 10128
Document the changed public defaults and update downstream users.
The constructor now treats omitted space-charge scales as non-overrides. LoadCDBInputs then loads Tpc_PolyClusterizer_kEff and, when available, applies the TpcConditions current correction. A missing kEff payload causes InitRun to return ABORTRUN.
The defaults also change from 380.0 to 375.0 V/cm and from 1.0 to -180.0 for both frame-charge scales. Add compatibility notes and update downstream macros or modules that depend on the previous defaults.
Source: Path instructions
| if (averageSR1 != 0.0 && averageNR1 != 0.0) | ||
| { | ||
| m_kEffSide0 *= m_conditions->get_LoadSR1() / averageSR1; | ||
| m_kEffSide1 *= m_conditions->get_LoadNR1() / averageNR1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Manual kEff values are scaled here, unlike in PHGarfield.
Lines 262-263 apply the current correction to m_kEffSide0 and m_kEffSide1 without checking m_kEffSide0Override / m_kEffSide1Override. A user who calls setKEffSide0() therefore does not get the value that was set. The printout at line 272 still labels it [manual value], so the discrepancy is not visible. PHGarfield::LoadCDBInputs lines 301-323 does guard each side with m_spaceChargeScaleOverride, so the two configuration paths now disagree for the same macro settings.
Proposed fix to match the PHGarfield behavior
if (averageSR1 != 0.0 && averageNR1 != 0.0)
{
- m_kEffSide0 *= m_conditions->get_LoadSR1() / averageSR1;
- m_kEffSide1 *= m_conditions->get_LoadNR1() / averageNR1;
+ if (!m_kEffSide0Override)
+ {
+ m_kEffSide0 *= m_conditions->get_LoadSR1() / averageSR1;
+ }
+ if (!m_kEffSide1Override)
+ {
+ m_kEffSide1 *= m_conditions->get_LoadNR1() / averageNR1;
+ }
}📝 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 (averageSR1 != 0.0 && averageNR1 != 0.0) | |
| { | |
| m_kEffSide0 *= m_conditions->get_LoadSR1() / averageSR1; | |
| m_kEffSide1 *= m_conditions->get_LoadNR1() / averageNR1; | |
| } | |
| if (averageSR1 != 0.0 && averageNR1 != 0.0) | |
| { | |
| if (!m_kEffSide0Override) | |
| { | |
| m_kEffSide0 *= m_conditions->get_LoadSR1() / averageSR1; | |
| } | |
| if (!m_kEffSide1Override) | |
| { | |
| m_kEffSide1 *= m_conditions->get_LoadNR1() / averageNR1; | |
| } | |
| } |
521579b
into
sPHENIX-Collaboration:master
2 participants
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
This PR updates the handling of TPC conditions and PHGarfield configuration used by the TPC PolyClusterizer.
Main changes:
TpcConditions(segment-by-segment).TpcConditionsare available.TPC_CONDITIONSis unavailable for a run, or theTpcConditionsnode is missing, reconstruction continues using the unscaled CDB kEff values and prints a warning instead of aborting.ConditionsAvailableto distinguish unavailable conditions from valid condition values.The updated
TpcConditions,PHGarfield, andTpc_PolyClusterizerpackages were tested successfully.The modified source files were also checked with
clang-tidyandclang-format.TODOs (if applicable)
Update CDB callibrations for kEffs.
Add TpcConditions to Macro
Links to other PRs in macros and calibration repositories (if applicable)
Motivation
Add time-dependent TPC condition handling to the TPC PolyClusterizer. Use CDB inputs by default while preserving explicit user configuration.
Key changes
PHGarfield.TpcConditionsare available.ConditionsAvailableand GEM-current accessors toTpcConditions.Potential risk areas
Future improvements
AI-generated summaries can contain errors. Contributors should verify the implementation and physics behavior against the source code and reference data.