reco error update#4176
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a runtime Changes
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}
Comment on lines
+533
to
692
| }else{ | ||
|
|
||
| if (TrkrDefs::getTrkrId(key) == TrkrDefs::tpcId) | ||
| { | ||
| if (layer == 7 || layer == 22 || layer == 23 || layer == 38 || layer == 39 || layer == 54) | ||
| { | ||
| phierror *= 4; | ||
| zerror *= 4; | ||
| } | ||
| if (cluster->getEdge() >= 3) | ||
| { | ||
| phierror *= 4; | ||
| } | ||
| if (cluster->getOverlap() >= 2) | ||
| { | ||
| phierror *= 2; | ||
| } | ||
| if(layer>=7&&layer<(7+48)){ | ||
| //Set phi error | ||
| if (cluster->getPhiSize() == 1) | ||
| { | ||
| phierror *= 1.0; | ||
| } | ||
| if (cluster->getPhiSize() == 2) | ||
| { | ||
| phierror*=3.15; | ||
| } | ||
| if (cluster->getPhiSize() == 3) | ||
| { | ||
| phierror *=3.5; | ||
| } | ||
| if (cluster->getPhiSize() >3) | ||
| { | ||
| phierror *= 4; | ||
| } | ||
| //Set Z Error | ||
| if (cluster->getZSize() == 1){ | ||
| zerror*=1.0; | ||
| } | ||
| if (cluster->getZSize() == 2){ | ||
| if(layer>=7&&layer<(7+16)){ | ||
| zerror*=7; | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*=4.5; | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*=4.5; | ||
| } | ||
|
|
||
| } | ||
| if ((cluster->getZSize() == 3) || (cluster->getZSize() == 4)){ | ||
| if(layer>=7&&layer<(7+16)){ | ||
| zerror*=7; | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*=5; | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*=5; | ||
| } | ||
| // zerror*=6; | ||
| } | ||
| if (cluster->getZSize() >5){ | ||
| if(layer>=7&&layer<(7+16)){ | ||
| zerror*=20; | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*=6; | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*=7; | ||
| } | ||
| } | ||
| TF1 ftpcR1("ftpcR1", "pol2", 0, 60); | ||
| ftpcR1.SetParameter(0, 3.206); | ||
| ftpcR1.SetParameter(1, -0.252); | ||
| ftpcR1.SetParameter(2, 0.007); | ||
|
|
||
| TF1 ftpcR2("ftpcR2", "pol2", 0, 60); | ||
| ftpcR2.SetParameter(0, 4.48); | ||
| ftpcR2.SetParameter(1, -0.226); | ||
| ftpcR2.SetParameter(2, 0.00362); | ||
|
|
||
| TF1 ftpcR3("ftpcR3", "pol2", 0, 60); | ||
| ftpcR3.SetParameter(0, 14.8112); | ||
| ftpcR3.SetParameter(1, -0.577); | ||
| ftpcR3.SetParameter(2, 0.00605); | ||
|
|
||
| if(layer>=7&&layer<(7+16)){ | ||
| phierror*= ftpcR1.Eval(layer); | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| phierror*= ftpcR2.Eval(layer); | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| phierror*= ftpcR3.Eval(layer); | ||
| } | ||
| ftpcR2.SetParameter(0, 5.593); | ||
| ftpcR2.SetParameter(1, -0.2458); | ||
| ftpcR2.SetParameter(2, 0.00333455); | ||
|
|
||
| ftpcR3.SetParameter(0, 5.6964); | ||
| ftpcR3.SetParameter(1, -0.21338); | ||
| ftpcR3.SetParameter(2, 0.002502); | ||
|
|
||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*= ftpcR2.Eval(layer); | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*= ftpcR3.Eval(layer); | ||
| } | ||
| } | ||
| if (cluster->getPhiSize() >= 5) | ||
| { | ||
| phierror *= 10; | ||
| } | ||
| } | ||
|
|
||
| if (TrkrDefs::getTrkrId(key) == TrkrDefs::mvtxId){ | ||
| phierror*=2; | ||
| zerror*=2; | ||
| } | ||
| if (cluster->getPhiSize() >= 5) | ||
| { | ||
| phierror *= 10; | ||
|
|
||
| if (TrkrDefs::getTrkrId(key) == TrkrDefs::inttId){ | ||
| phierror*=9; | ||
| if (cluster->getPhiSize() == 1){ | ||
| phierror *= 1.25; | ||
| } | ||
| if (cluster->getPhiSize() == 2){ | ||
| phierror *= 2.25; | ||
| } | ||
| if(layer==3||layer==4) | ||
| phierror*=0.8; | ||
| if(layer==5||layer==6) | ||
| phierror*=1.2; | ||
| } | ||
|
|
||
| phierror = std::min(phierror, 0.1); | ||
| if (phierror < 0.0005) | ||
| { | ||
| phierror = 0.1; | ||
|
|
||
| if (TrkrDefs::getTrkrId(key) == TrkrDefs::micromegasId){ | ||
| if(layer==55){ | ||
| /* | ||
| phierror*=5.4; | ||
| phierror*=4.6; | ||
| phierror*=3.0; | ||
| zerror*=0.82; | ||
| */ | ||
| phierror = 0.0289; | ||
| } | ||
|
|
||
| if(layer==56){ | ||
| /* | ||
| phierror*=0.9; | ||
| phierror*=0.95; | ||
| zerror*=4.5; | ||
| zerror*=3.4; | ||
| */ | ||
| zerror = 0.577; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Data-reco TPC path is missing the phierror floor/ceiling clamp present in the MC path.
The MC branch (lines 527–531) enforces phierror = std::min(phierror, 0.1) and floors small values to 0.1. The data branch has no equivalent bounds check for TPC phierror or zerror, so extreme values (very small errors from the polynomial, or very large accumulated multipliers) can propagate unchecked. Verify this is intentional — unbounded errors can destabilize track fitting.
Comment on lines
25
to
34
| ClusterErrorPara::ClusterErrorPara(): f0{new TF1("f0", "pol1", 0, 10)} | ||
| { | ||
| /* | ||
| ftpcR1 = new TF1("ftpcR1", "pol2", 0, 10); | ||
| ftpcR1->SetParameter(0, 3.206); | ||
| ftpcR1->SetParameter(1, -0.252); | ||
| ftpcR1->SetParameter(2, 0.007); | ||
| */ | ||
|
|
||
| f0 = new TF1("f0", "pol1", 0, 10); |
Contributor
There was a problem hiding this comment.
Memory leak: f0 is allocated twice — once in the member-initializer list, once in the constructor body.
Line 25 allocates f0 via the member-init list (f0{new TF1(...)}), then line 34 allocates a second TF1 and overwrites the pointer, leaking the first. Each leaked TF1 also registers itself in ROOT's gDirectory.
Remove the member-initializer allocation and keep only the body assignment (which sets the parameters), or vice versa.
Proposed fix
-ClusterErrorPara::ClusterErrorPara(): f0{new TF1("f0", "pol1", 0, 10)}
+ClusterErrorPara::ClusterErrorPara()
{
- /*
- ftpcR1 = new TF1("ftpcR1", "pol2", 0, 10);
- ftpcR1->SetParameter(0, 3.206);
- ftpcR1->SetParameter(1, -0.252);
- ftpcR1->SetParameter(2, 0.007);
- */
-
f0 = new TF1("f0", "pol1", 0, 10);
Comment on lines
+546
to
+650
| if (cluster->getOverlap() >= 2) | ||
| { | ||
| phierror *= 2; | ||
| } | ||
| if(layer>=7&&layer<(7+48)){ | ||
| //Set phi error | ||
| if (cluster->getPhiSize() == 1) | ||
| { | ||
| phierror *= 1.0; | ||
| } | ||
| if (cluster->getPhiSize() == 2) | ||
| { | ||
| phierror*=3.15; | ||
| } | ||
| if (cluster->getPhiSize() == 3) | ||
| { | ||
| phierror *=3.5; | ||
| } | ||
| if (cluster->getPhiSize() >3) | ||
| { | ||
| phierror *= 4; | ||
| } | ||
| //Set Z Error | ||
| if (cluster->getZSize() == 1){ | ||
| zerror*=1.0; | ||
| } | ||
| if (cluster->getZSize() == 2){ | ||
| if(layer>=7&&layer<(7+16)){ | ||
| zerror*=7; | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*=4.5; | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*=4.5; | ||
| } | ||
|
|
||
| } | ||
| if ((cluster->getZSize() == 3) || (cluster->getZSize() == 4)){ | ||
| if(layer>=7&&layer<(7+16)){ | ||
| zerror*=7; | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*=5; | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*=5; | ||
| } | ||
| // zerror*=6; | ||
| } | ||
| if (cluster->getZSize() >5){ | ||
| if(layer>=7&&layer<(7+16)){ | ||
| zerror*=20; | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*=6; | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*=7; | ||
| } | ||
| } | ||
| TF1 ftpcR1("ftpcR1", "pol2", 0, 60); | ||
| ftpcR1.SetParameter(0, 3.206); | ||
| ftpcR1.SetParameter(1, -0.252); | ||
| ftpcR1.SetParameter(2, 0.007); | ||
|
|
||
| TF1 ftpcR2("ftpcR2", "pol2", 0, 60); | ||
| ftpcR2.SetParameter(0, 4.48); | ||
| ftpcR2.SetParameter(1, -0.226); | ||
| ftpcR2.SetParameter(2, 0.00362); | ||
|
|
||
| TF1 ftpcR3("ftpcR3", "pol2", 0, 60); | ||
| ftpcR3.SetParameter(0, 14.8112); | ||
| ftpcR3.SetParameter(1, -0.577); | ||
| ftpcR3.SetParameter(2, 0.00605); | ||
|
|
||
| if(layer>=7&&layer<(7+16)){ | ||
| phierror*= ftpcR1.Eval(layer); | ||
| } | ||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| phierror*= ftpcR2.Eval(layer); | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| phierror*= ftpcR3.Eval(layer); | ||
| } | ||
| ftpcR2.SetParameter(0, 5.593); | ||
| ftpcR2.SetParameter(1, -0.2458); | ||
| ftpcR2.SetParameter(2, 0.00333455); | ||
|
|
||
| ftpcR3.SetParameter(0, 5.6964); | ||
| ftpcR3.SetParameter(1, -0.21338); | ||
| ftpcR3.SetParameter(2, 0.002502); | ||
|
|
||
| if(layer>=(7+16)&&layer<(7+32)){ | ||
| zerror*= ftpcR2.Eval(layer); | ||
| } | ||
| if(layer>=(7+32)&&layer<(7+48)){ | ||
| zerror*= ftpcR3.Eval(layer); | ||
| } | ||
| } | ||
| if (cluster->getPhiSize() >= 5) | ||
| { | ||
| phierror *= 10; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
phiSize >= 5 gets a compounded 40× multiplier — likely unintentional double-hit.
Inside the layer>=7 && layer<55 block, line 564 applies phierror *= 4 for phiSize > 3 (which includes phiSize ≥ 5). Then outside that block but still inside the tpcId scope, line 646 applies phierror *= 10 for phiSize >= 5. The two multiply together yielding 40× for large clusters, whereas phiSize == 4 only gets 4×.
This 10× discontinuity between phiSize 4 and 5 (after TF1 factors) is suspicious. If both conditions are intentional, they should be documented; otherwise, the inner condition at line 564 should use == 4 or the outer check at line 646 should be removed.
If the intent is a single 10× for phiSize≥5 (matching MC branch pattern)
- if (cluster->getPhiSize() >3)
+ if (cluster->getPhiSize() == 4)
{
phierror *= 4;
}
Build & test reportReport for commit f34b4423c5e8e2d12394c46a3f52fd9ac84f397c:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit e13fbb091d87c78fdadf87e397640bad2df84d07:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 3d74e878218b940240de2922392be3fd02545830:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 0ae0e709d54e7f8ae674289c036dff0f79a4778a:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit ee1177c011476e8f7a8d535dd08ea25b3a5d04af:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit e18101b7274693507df186e239b1d9751a6ea263:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit e11adf0566fe8cfa6db382c1499077fabf2d29cd:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 4c4088d2cf901636843a4fdfb79731a895e818f4:
Automatically generated by sPHENIX Jenkins continuous integration |
4 tasks
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, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Cluster Error Parametrization Update for Data vs. MC Reconstruction
Motivation / Context
Introduce a runtime distinction between real data and Monte‑Carlo reconstruction so cluster error parametrizations can apply detector- and reconstruction‑mode specific scalings. This allows different phi/z error treatments for data vs MC while preserving existing public interfaces.
Key changes
Potential risk areas
Possible future improvements
Note: This summary was produced with AI assistance. Please verify numeric values and code details carefully — AI can make mistakes and may misinterpret subtle implementation specifics.