add option to not do rescaling in RetowerCEMC#4283
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesConfigurable tower energy rescaling
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/jetbackground/RetowerCEMC.ccIn file included from offline/packages/jetbackground/RetowerCEMC.cc:1: ... [truncated 1200 characters] ... nclude" 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8b65300-6d78-4641-b28a-f4263d9aadc8
📒 Files selected for processing (2)
offline/packages/jetbackground/RetowerCEMC.ccoffline/packages/jetbackground/RetowerCEMC.h
Comment on lines
+148
to
+152
| if(_do_rescale) | ||
| { | ||
| towerinfo->set_energy(retower_e_temp / (double) (1 - scalefactor)); | ||
| } | ||
| else towerinfo->set_energy(retower_e_temp); |
Contributor
There was a problem hiding this comment.
Division by zero when scalefactor == 1.0 and rescaling is enabled.
If a retower bin has 100% bad area (scalefactor = 1.0) and _do_rescale is true, line 150 will divide by zero: retower_e_temp / (1 - 1) = retower_e_temp / 0, producing NaN or inf. This case is reachable because:
- The masking condition at line 141 is
scalefactor > _frac_cut. With the new default_frac_cut = 1, a tower with exactly scalefactor = 1 passes through to the rescaling branch. - When all contributing EMCal towers in a bin are bad,
scalefactor = retower_badarea / retower_totalarea = 1.
Add a guard to prevent division by zero or by values very close to zero.
🛡️ Proposed fix to guard against division by zero
else
{
if(_do_rescale)
{
- towerinfo->set_energy(retower_e_temp / (double) (1 - scalefactor));
+ if (scalefactor < 1.0)
+ {
+ towerinfo->set_energy(retower_e_temp / (double) (1 - scalefactor));
+ }
+ else
+ {
+ towerinfo->set_energy(0); // or handle as masked tower
+ }
}
else towerinfo->set_energy(retower_e_temp);Alternatively, consider whether towers with scalefactor approaching 1 should be masked rather than rescaled.
📝 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.
Suggested change
| if(_do_rescale) | |
| { | |
| towerinfo->set_energy(retower_e_temp / (double) (1 - scalefactor)); | |
| } | |
| else towerinfo->set_energy(retower_e_temp); | |
| if(_do_rescale) | |
| { | |
| if (scalefactor < 1.0) | |
| { | |
| towerinfo->set_energy(retower_e_temp / (double) (1 - scalefactor)); | |
| } | |
| else | |
| { | |
| towerinfo->set_energy(0); // or handle as masked tower | |
| } | |
| } | |
| else towerinfo->set_energy(retower_e_temp); |
Comment on lines
+36
to
+37
| double _frac_cut{1}; | ||
| bool _do_rescale{false}; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the impact of the _frac_cut default change on downstream tower usage
# Search for how the retower output is consumed and whether masking behavior matters
rg -nP --type=cc -C3 'TOWER_CALIB_CEMC_RETOWER|CEMC_RETOWER' -g '!RetowerCEMC.cc'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate and inspect RetowerCEMC.h around the relevant lines
REPO_ROOT="$(pwd)"
FILE="$(fd -a --full-path 'RetowerCEMC.h' . | head -n 1 || true)"
echo "Found RetowerCEMC.h: $FILE"
if [[ -n "${FILE:-}" ]]; then
echo "---- RetowerCEMC.h lines 1-120 ----"
nl -ba "$FILE" | sed -n '1,120p'
fi
# 2) Find all usages of _frac_cut and masking logic
if [[ -n "${FILE:-}" ]]; then
echo "---- _frac_cut usage ----"
rg -n "_frac_cut" "$FILE" -C2 || true
echo "---- _frac_cut usage across repo (limited to likely files) ----"
rg -n "_frac_cut" -S . | head -n 200
fi
# 3) Find strings for retower outputs and downstream consumption
echo "---- Search for retower output consumers ----"
rg -n --hidden --no-ignore-vcs -S 'TOWER_CALIB_CEMC_RETOWER|CEMC_RETOWER' . | head -n 200Repository: sPHENIX-Collaboration/coresoftware
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate and inspect RetowerCEMC.h around the relevant lines
REPO_ROOT="$(pwd)"
FILE="$(fd -a --full-path 'RetowerCEMC.h' . | head -n 1 || true)"
echo "Found RetowerCEMC.h: $FILE"
if [[ -n "${FILE:-}" ]]; then
echo "---- RetowerCEMC.h lines 1-120 ----"
nl -ba "$FILE" | sed -n '1,120p'
fi
# 2) Find all usages of _frac_cut and masking logic
if [[ -n "${FILE:-}" ]]; then
echo "---- _frac_cut usage ----"
rg -n "_frac_cut" "$FILE" -C2 || true
echo "---- _frac_cut usage across repo (limited to likely files) ----"
rg -n "_frac_cut" -S . | head -n 200
fi
# 3) Find strings for retower outputs and downstream consumption
echo "---- Search for retower output consumers ----"
rg -n --hidden --no-ignore-vcs -S 'TOWER_CALIB_CEMC_RETOWER|CEMC_RETOWER' . | head -n 200Repository: sPHENIX-Collaboration/coresoftware
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -a --full-path 'RetowerCEMC.h' . | head -n 1 || true)"
echo "Found RetowerCEMC.h: $FILE"
if [[ -n "${FILE:-}" ]]; then
echo "---- RetowerCEMC.h lines 1-160 ----"
nl -ba "$FILE" | sed -n '1,160p'
fi
if [[ -n "${FILE:-}" ]]; then
echo "---- _frac_cut usage (within file) ----"
rg -n "_frac_cut" "$FILE" -C2 || true
fi
echo "---- _frac_cut usage across repo (first 200 hits) ----"
rg -n "_frac_cut" -S . | head -n 200 || true
echo "---- Retower output identifiers usage (first 200 hits) ----"
rg -n --hidden --no-ignore-vcs -S 'TOWER_CALIB_CEMC_RETOWER|CEMC_RETOWER' . | head -n 200 || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -a --full-path 'RetowerCEMC.h' . | head -n 1 || true)"
echo "Found RetowerCEMC.h: $FILE"
if [[ -n "${FILE:-}" ]]; then
echo "---- RetowerCEMC.h excerpt (1-220) with line numbers ----"
# Use cat -n (available) instead of nl
cat -n "$FILE" | sed -n '1,220p'
echo "---- _frac_cut usage (within file) ----"
rg -n "_frac_cut" "$FILE" -C3 || true
fi
echo "---- Retower output identifier usage (first 200 hits) ----"
rg -n --hidden --no-ignore-vcs -S 'TOWER_CALIB_CEMC_RETOWER|CEMC_RETOWER' . | head -n 200 || true
echo "---- Any masking-related usage nearby ----"
rg -n --hidden --no-ignore-vcs -S 'mask|masked|bad.*frac|frac_cut|rescale|scalefactor' offline/packages/jetbackground/RetowerCEMC.h 2>/dev/null || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 10758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# locate RetowerCEMC.cc
FILE="$(fd -a --full-path 'RetowerCEMC.cc' . | head -n 1 || true)"
echo "Found RetowerCEMC.cc: $FILE"
if [[ -n "${FILE:-}" ]]; then
echo "---- RetowerCEMC.cc excerpt around _frac_cut ----"
rg -n "_frac_cut" "$FILE" -n -C3 || true
echo "---- Search for scalefactor/rescale/mask/bad area logic ----"
rg -n --hidden --no-ignore-vcs -S 'scalefactor|rescale|rescal|frac_cut|bad.*area|mask|masked|badFrac|fraction' "$FILE" -C2 || true
echo "---- Show function(s) that likely use _frac_cut (first few matches) ----"
# Grab some broader context around the first occurrences
FIRST=$(rg -n "_frac_cut" "$FILE" | head -n 1 | cut -d: -f1 || true)
if [[ -n "${FIRST:-}" ]]; then
START=$((FIRST-80))
END=$((FIRST+120))
if (( START<1 )); then START=1; fi
echo "---- RetowerCEMC.cc lines $START-$END ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fi
fi
echo "---- Inspect likely downstream usage: JetBackgroundCut.h and DetermineTowerBackground.cc ----"
for f in offline/packages/jetbackground/JetBackgroundCut.h offline/packages/jetbackground/DetermineTowerBackground.cc offline/packages/jetbase/TowerJetInput.cc; do
if [[ -f "$f" ]]; then
echo "### $f"
rg -n --no-ignore-vcs -S 'TOWERINFO_CALIB_CEMC_RETOWER|TOWER_CALIB_CEMC_RETOWER|m_input_cemc_tower_node|mask|masked|bad|frac_cut|towerinfo' "$f" -C2 || true
fi
doneRepository: sPHENIX-Collaboration/coresoftware
Length of output: 35273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -a --full-path 'RetowerCEMC.cc' . | head -n 1 || true)"
echo "RetowerCEMC.cc: $FILE"
if [[ -n "${FILE:-}" ]]; then
# show sections around get_fraction and get_weighted_fraction and retower_totalarea computation
# We'll locate "void RetowerCEMC::get_fraction" line numbers
EF=$(rg -n "void RetowerCEMC::get_fraction" "$FILE" | cut -d: -f1 | head -n1 || true)
EW=$(rg -n "void RetowerCEMC::get_weighted_fraction" "$FILE" | cut -d: -f1 | head -n1 || true)
echo "get_fraction starts at: ${EF:-N/A}, get_weighted_fraction starts at: ${EW:-N/A}"
if [[ -n "${EF:-}" ]]; then
START=$((EF-40)); END=$((EF+140))
(( START<1 )) && START=1
echo "---- RetowerCEMC.cc excerpt around get_fraction ($START-$END) ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fi
if [[ -n "${EW:-}" ]]; then
START=$((EW-40)); END=$((EW+220))
(( START<1 )) && START=1
echo "---- RetowerCEMC.cc excerpt around get_weighted_fraction ($START-$END) ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fi
fi
echo "---- Locate TowerInfo definitions for set_isHot/get_isGood ----"
# Find TowerInfo.h/cc and relevant methods
TIH="$(fd -a --full-path 'TowerInfo*' . | rg -m 1 -v '/src/' || true)"
echo "Some TowerInfo* match (first): $TIH"
# Use ripgrep for set_isHot and get_isGood in headers
rg -n --hidden --no-ignore-vcs -S 'set_isHot\(|get_isGood\(' . | head -n 50 || true
# Specifically locate TowerInfo class header if present
TOWERINFO_H=$(fd -a --full-path 'TowerInfo.h' . | head -n 5 || true)
echo "TowerInfo.h candidates:"
echo "$TOWERINFO_H" | head -n 5
for f in $TOWERINFO_H; do
echo "---- excerpt from $f ----"
rg -n "class TowerInfo|set_isHot|get_isGood" "$f" -n -C2 || true
doneRepository: sPHENIX-Collaboration/coresoftware
Length of output: 18685
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -a --full-path 'RetowerCEMC.cc' . | head -n 1 || true)"
echo "RetowerCEMC.cc: $FILE"
if [[ -n "${FILE:-}" ]]; then
EF=$(rg -n "void RetowerCEMC::get_fraction" "$FILE" | cut -d: -f1 | head -n1 || true)
EW=$(rg -n "void RetowerCEMC::get_weighted_fraction" "$FILE" | cut -d: -f1 | head -n1 || true)
echo "get_fraction starts at: ${EF:-N/A}, get_weighted_fraction starts at: ${EW:-N/A}"
if [[ -n "${EF:-}" ]]; then
START=$((EF-40)); END=$((EF+160))
(( START<1 )) && START=1
echo "---- RetowerCEMC.cc excerpt around get_fraction ($START-$END) ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fi
if [[ -n "${EW:-}" ]]; then
START=$((EW-40)); END=$((EW+260))
(( START<1 )) && START=1
echo "---- RetowerCEMC.cc excerpt around get_weighted_fraction ($START-$END) ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fi
fi
echo "---- Locate TowerInfo definitions for set_isHot/get_isGood ----"
# Search for the methods across the repo (limit output)
rg -n --hidden --no-ignore-vcs -S 'set_isHot\(|get_isGood\(' . | head -n 80 || true
# Try common header name
TOWERINFO_H=$(fd -a --full-path 'TowerInfo*.h' . | head -n 20 || true)
echo "TowerInfo*.h candidates (up to 20):"
echo "$TOWERINFO_H" | head -n 20
for f in $TOWERINFO_H; do
if [[ -f "$f" ]]; then
if rg -q "class TowerInfo" "$f" || rg -q "set_isHot" "$f" || rg -q "get_isGood" "$f"; then
echo "---- relevant excerpt from $f ----"
rg -n "set_isHot|get_isGood" "$f" -C2 || true
fi
fi
doneRepository: sPHENIX-Collaboration/coresoftware
Length of output: 18443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -a --full-path 'RetowerCEMC.cc' . | head -n 1 || true)"
echo "RetowerCEMC.cc: $FILE"
if [[ -n "${FILE:-}" ]]; then
EF=$(rg -n "void RetowerCEMC::get_fraction" "$FILE" | cut -d: -f1 | head -n1 || true)
EW=$(rg -n "void RetowerCEMC::get_weighted_fraction" "$FILE" | cut -d: -f1 | head -n1 || true)
echo "get_fraction starts at: ${EF:-N/A}, get_weighted_fraction starts at: ${EW:-N/A}"
if [[ -n "${EF:-}" ]]; then
START=$((EF-40)); END=$((EF+160))
(( START<1 )) && START=1
echo "---- RetowerCEMC.cc excerpt around get_fraction ($START-$END) ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fi
if [[ -n "${EW:-}" ]]; then
START=$((EW-40)); END=$((EW+260))
(( START<1 )) && START=1
echo "---- RetowerCEMC.cc excerpt around get_weighted_fraction ($START-$END) ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fi
fi
echo "---- Locate TowerInfo definitions for set_isHot/get_isGood ----"
# Search for the methods across the repo (limit output)
rg -n --hidden --no-ignore-vcs -S 'set_isHot\(|get_isGood\(' . | head -n 80 || true
# Try common header name
TOWERINFO_H=$(fd -a --full-path 'TowerInfo*.h' . | head -n 20 || true)
echo "TowerInfo*.h candidates (up to 20):"
echo "$TOWERINFO_H" | head -n 20
for f in $TOWERINFO_H; do
if [[ -f "$f" ]]; then
if rg -q "class TowerInfo" "$f" || rg -q "set_isHot" "$f" || rg -q "get_isGood" "$f"; then
echo "---- relevant excerpt from $f ----"
rg -n "set_isHot|get_isGood" "$f" -C2 || true
fi
fi
doneRepository: sPHENIX-Collaboration/coresoftware
Length of output: 18443
Major: Raise of _frac_cut default to 1.0 effectively disables retower masking (behavioral impact)
double _frac_cut{1};
bool _do_rescale{false};In offline/packages/jetbackground/RetowerCEMC.cc, a (retower) tower is masked only when scalefactor = retower_badarea / retower_totalarea[ieta_ihcal] satisfies scalefactor > _frac_cut; then the code sets towerinfo->set_energy(0) and towerinfo->set_isHot(true) (which makes tower->get_isGood() false in TowerInfov2/4). With _frac_cut now defaulting to 1.0, masking should only occur if scalefactor exceeds ~1 due to numerical effects, so high-bad-area towers will generally remain unmasked and therefore propagate through downstream steps like offline/packages/jetbackground/DetermineTowerBackground.cc, which excludes masked towers via !towerinfo->get_isGood() (and also references E==0 exclusion).
Confirm this intended change in default behavior and that downstream analyses expect towers with large bad-area fractions to remain included; PR description should explicitly state analysis impact and whether reprocessing is required.
Build & test reportReport for commit 727d732973ee35d1d373fc7d257681d3383a3883:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 7d93cb5d96875dbe87f870924c66912edaccd7e7:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 8d386e95729cf8c5a55840b1db142ed8ce4f6e98:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 25eecda311902299ca488b54b7e02fe361266675:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
|
the valgrind error is from the new acts version and needs to be suppressed |
pinkenburg
merged commit 9128ec0
into
sPHENIX-Collaboration:master
21 of 22 checks passed
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, ...)
Adding flag to not rescale EMCal retower energy by fraction of dead area. See details of the problems this causes in Dennis's presentation here: https://indico.bnl.gov/event/33114/
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
HIJetReco update coming
Motivation / Context
EMCal retowering previously unconditionally rescaled retowered energy by the fraction of good area when towers exceeded a dead-area threshold. That behavior can bias reconstruction in regions with dead towers; this PR introduces a configurable option to disable that rescaling so groups can choose the reconstruction strategy that matches their analyses (motivated by Dennis's presentation).
Note: this summary was produced with AI assistance — AI can make mistakes; please verify details against the code and tests.
Key changes
Potential risk areas
Possible future improvements