New setters for lamination fitting code#4270
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughTpcLaminationFitting adds public configuration methods for histogram binning, SD-layer cut behavior, and ADC weighting. Private state stores these settings. InitRun uses configurable bin dimensions for histogram creation. Event processing applies optional ADC-derived weights to fills and conditionally applies SD-layer selection based on the configuration flags. ChangesLamination Fitting Configuration and Weighting
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c2d59e9-30fd-418b-afec-f0d176ccca36
📒 Files selected for processing (2)
offline/packages/tpccalib/TpcLaminationFitting.ccoffline/packages/tpccalib/TpcLaminationFitting.h
Comment on lines
+61
to
+65
| void TpcLaminationFitting::set_lam_grid_dimensions(int phibins, int rbins) | ||
| { | ||
| m_lamPhiBins = phibins; | ||
| m_lamRBins = rbins; | ||
| } |
Contributor
There was a problem hiding this comment.
Validate lamination grid dimensions before storing them.
Line 61 accepts unchecked bin counts, and Lines 101/109 use them directly in TH2D allocation. Non-positive or extreme values can create invalid histograms or trigger large memory spikes.
Proposed fix
void TpcLaminationFitting::set_lam_grid_dimensions(int phibins, int rbins)
{
- m_lamPhiBins = phibins;
- m_lamRBins = rbins;
+ constexpr int kMinBins = 1;
+ constexpr int kMaxBins = 2000;
+ if (phibins < kMinBins || rbins < kMinBins || phibins > kMaxBins || rbins > kMaxBins)
+ {
+ std::cout << PHWHERE
+ << " invalid lamination grid dimensions (phi=" << phibins
+ << ", r=" << rbins << "), keeping previous values (phi="
+ << m_lamPhiBins << ", r=" << m_lamRBins << ")"
+ << std::endl;
+ return;
+ }
+ m_lamPhiBins = phibins;
+ m_lamRBins = rbins;
}As per coding guidelines, **/*.{cc,cpp,cxx,c} files should prioritize correctness and error handling.
📝 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
| void TpcLaminationFitting::set_lam_grid_dimensions(int phibins, int rbins) | |
| { | |
| m_lamPhiBins = phibins; | |
| m_lamRBins = rbins; | |
| } | |
| void TpcLaminationFitting::set_lam_grid_dimensions(int phibins, int rbins) | |
| { | |
| constexpr int kMinBins = 1; | |
| constexpr int kMaxBins = 2000; | |
| if (phibins < kMinBins || rbins < kMinBins || phibins > kMaxBins || rbins > kMaxBins) | |
| { | |
| std::cout << PHWHERE | |
| << " invalid lamination grid dimensions (phi=" << phibins | |
| << ", r=" << rbins << "), keeping previous values (phi=" | |
| << m_lamPhiBins << ", r=" << m_lamRBins << ")" | |
| << std::endl; | |
| return; | |
| } | |
| m_lamPhiBins = phibins; | |
| m_lamRBins = rbins; | |
| } |
Build & test reportReport for commit f919460f19f1c0d100eb3fae03e3eadc88e7d07c:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
|
clang-tidy warnings are from another PR which fixed those |
pinkenburg
merged commit c416f2f
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, ...)
New setter functions to allow for easy toggling of layer standard deviation cut and adc weighting of histograms. Also new function to set the number of phi and R bins of the lamination histograms. All setters default to the same settings as before.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation
This PR adds three new configurable setter methods to the TPC lamination fitting calibration code, enabling physics analysts to tune lamination histogram binning and adjust event selection and weighting strategies without code modifications. These settings control how lamination distortions are measured and corrected for the TPC detector's space charge effects. The changes maintain backward compatibility by preserving existing default behavior.
Key Changes
Lamination histogram grid configuration: New
set_lam_grid_dimensions(int phibins, int rbins)setter allows customization of lamination histogram bin counts in the φ (azimuthal) and R (radial) dimensions (default: 200 bins each). Previously hardcoded, these dimensions are now applied duringInitRun()for all 18 laminations on both TPC sides (lines 101, 109 in TpcLaminationFitting.cc).Structured digit (SD) layer cut toggle: New
set_useSDLayerCut(bool useCut)setter enables/disables SD-layer-based filtering (default:trueto maintain existing behavior). The logic is inverted from typical naming:m_useSDLayerCut = true: Clusters with high SD-weighted layer values (> 0.5) are excluded from petal histogram filling (line 248-250) but included in lamination filling (line 225)m_useSDLayerCut = false: All clusters pass through to petal histogram filling without SD filteringADC-weighted histogram filling: New
set_adcWeight(bool useADC)setter enables optional cluster ADC-value-based event weighting (default:false). When enabled, each cluster's ADC value is used as a weight when filling both lamination and petal histograms (lines 206-209, 243, 267).Member variables added:
m_useSDLayerCut,m_adcWeight,m_lamPhiBins, andm_lamRBinswith appropriate defaults.Potential Risk Areas
Non-intuitive SD-layer cut logic: The
m_useSDLayerCutflag performs different filtering for laminations (includes high-SD clusters) versus petal histograms (excludes high-SD clusters). This asymmetric behavior could lead to unexpected results if analysts misunderstand the intent.Reconstruction behavior changes: The conditional SD-layer filtering for petal histogram affects which clusters contribute to the global R-matching and overall distortion correction strategy, potentially impacting the final distortion maps when the flag is toggled.
ADC weighting impact: The optional ADC weighting changes the statistical weight of events and could affect lamination fitting convergence, especially for events with very high or very low ADC values. Unvalidated ADC weighting configurations could bias the extracted corrections.
Histogram resolution sensitivity: Different phi/R bin dimensions directly affect histogram granularity and fitting precision. Under-binned histograms may increase statistical noise in fits; over-binned histograms may lose spatial resolution of lamination features.
Silent configuration dependency: Default-disabled features may lead to subtle behavior changes if users forget to enable ADC weighting when expected, or if different analyses use different
m_useSDLayerCutsettings without documentation.Possible Future Improvements
m_useSDLayerCutto better convey the filtering logic (e.g.,m_filterHighSDClusters)Note: This summary is AI-generated and may contain inaccuracies. The asymmetric SD-layer filtering logic (different behavior for laminations vs. petal histograms) warrants careful review with collaboration experts to confirm the intended physics behavior.