fix clusterizer to handle negative charge #20555
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @dkotlins for master. It involves the following packages: RecoLocalTracker/SiPixelClusterizer @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| #ifdef PIXELREGRESSION | ||
| int adcOld = calibrate(di->adc(),col,row); | ||
| //assert(adc==adcOld); | ||
| if (adc!=adcOld) std::cout << "VI " << eqD <<' '<< ic <<' '<< end-begin <<' '<< i <<' '<< di->adc() <<' ' << adc <<' '<< adcOld << std::endl; else ++eqD; | ||
| #endif | ||
|
|
||
| //if(adc<0) cout<<" negative amplitude "<<adc<<" "<<row<<" "<<col<<" "<<calibrate(di->adc(),col,row)<<" "<<detid_<<endl; |
There was a problem hiding this comment.
Please remove unneded commented out code
| float DBpedestal = theSiPixelGainCalibrationService_->getPedestal(detid_, col, row) * DBgain; | ||
| float pedestal = theSiPixelGainCalibrationService_->getPedestal(detid_, col, row); | ||
| float DBpedestal = pedestal * DBgain; | ||
| //float DBpedestal = theSiPixelGainCalibrationService_->getPedestal(detid_, col, row) * DBgain; |
There was a problem hiding this comment.
Please remove unneded commented out code
| @@ -351,6 +358,26 @@ int PixelThresholdClusterizer::calibrate(int adc, int col, int row) | |||
| } else { | |||
| electrons = int( vcal * theConversionFactor + theOffset); | |||
| } | |||
|
|
|||
|
|
|||
| // if(adc<0 || adc>255) cout<<" adc wrong range "<<adc<<endl; | |||
There was a problem hiding this comment.
Please remove unneded commented out code, or add a comment which explains why it is needed here
| // } | ||
|
|
||
| if(electrons<0) | ||
| cout<<" layer "<<layer_<<" "<<adc<<" "<<DBgain<<" "<<pedestal<<" "<<DBpedestal<<" "<<vcal |
There was a problem hiding this comment.
cout is not thread safe
I suspect anyhow that also this line was originally supposed to get commented out
|
Can’t we have any debug code any more? If something goes wrong how do we debug?
Acturally the ifdef PIXEL_REGRESSION is very old and come from Vincenzo I think.
It has been in the code for 5-6 years and nobody has complained until now!
D.
… On 18 Sep 2017, at 12:04, perrotta ***@***.***> wrote:
@perrotta commented on this pull request.
In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
> #ifdef PIXELREGRESSION
int adcOld = calibrate(di->adc(),col,row);
//assert(adc==adcOld);
if (adc!=adcOld) std::cout << "VI " << eqD <<' '<< ic <<' '<< end-begin <<' '<< i <<' '<< di->adc() <<' ' << adc <<' '<< adcOld << std::endl; else ++eqD;
#endif
+
+ //if(adc<0) cout<<" negative amplitude "<<adc<<" "<<row<<" "<<col<<" "<<calibrate(di->adc(),col,row)<<" "<<detid_<<endl;
Please remove unneded commented out code
In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
> @@ -324,7 +329,9 @@ int PixelThresholdClusterizer::calibrate(int adc, int col, int row)
//const float pedestal = -28.2 * gain; // -79.
float DBgain = theSiPixelGainCalibrationService_->getGain(detid_, col, row);
- float DBpedestal = theSiPixelGainCalibrationService_->getPedestal(detid_, col, row) * DBgain;
+ float pedestal = theSiPixelGainCalibrationService_->getPedestal(detid_, col, row);
+ float DBpedestal = pedestal * DBgain;
+ //float DBpedestal = theSiPixelGainCalibrationService_->getPedestal(detid_, col, row) * DBgain;
Please remove unneded commented out code
In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
> @@ -351,6 +358,26 @@ int PixelThresholdClusterizer::calibrate(int adc, int col, int row)
} else {
electrons = int( vcal * theConversionFactor + theOffset);
}
+
+
+ // if(adc<0 || adc>255) cout<<" adc wrong range "<<adc<<endl;
Please remove unneded commented out code, or add a comment which explains why it is needed here
In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
> + // if(adc<0 || adc>255) cout<<" adc wrong range "<<adc<<endl;
+
+ // if(layer_>0) {
+ // if( (layer_==1 && (pedestal<-40. || pedestal>170.) ) ||
+ // (layer_==2 && (pedestal<-260.|| pedestal>230.) ) ||
+ // (layer_==3 && (pedestal<-130.|| pedestal>80.) ) ||
+ // (layer_==4 && (pedestal<-90. || pedestal>110.) ) )
+
+ // cout<<"PED outside range: layer "<<layer_<<" "<<pedestal<<" "<<DBgain<<" "<<DBpedestal
+ // <<" "<<adc<<" "<<vcal<<" "<<electrons<<" det: "
+ // <<detid_<<" row: "<<row<<" col: "<<col<<endl;
+
+ // }
+
+ if(electrons<0)
+ cout<<" layer "<<layer_<<" "<<adc<<" "<<DBgain<<" "<<pedestal<<" "<<DBpedestal<<" "<<vcal
cout is not thread safe
I suspect anyhow that also this line was originally supposed to get commented out
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@dkotlins : I didn't comment about "ifdef PIXEL_REGRESSION", only
about the new lines you added to the code.
Commented out lines should better be avoided. Alternatively, another
line of comments should explain why they are needed to be kept in the
code.
dkotlins <notifications@github.com> ha scritto:
… Can’t we have any debug code any more? If something goes wrong how
do we debug?
Acturally the ifdef PIXEL_REGRESSION is very old and come from
Vincenzo I think.
It has been in the code for 5-6 years and nobody has complained until now!
D.
> On 18 Sep 2017, at 12:04, perrotta ***@***.***> wrote:
>
> @perrotta commented on this pull request.
>
> In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
>
> > #ifdef PIXELREGRESSION
> int adcOld = calibrate(di->adc(),col,row);
> //assert(adc==adcOld);
> if (adc!=adcOld) std::cout << "VI " << eqD <<' '<< ic <<'
> '<< end-begin <<' '<< i <<' '<< di->adc() <<' ' << adc <<' '<<
> adcOld << std::endl; else ++eqD;
> #endif
> +
> + //if(adc<0) cout<<" negative amplitude "<<adc<<" "<<row<<"
> "<<col<<" "<<calibrate(di->adc(),col,row)<<" "<<detid_<<endl;
>
> Please remove unneded commented out code
>
> In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
>
> > @@ -324,7 +329,9 @@ int PixelThresholdClusterizer::calibrate(int
> adc, int col, int row)
> //const float pedestal = -28.2 * gain; // -79.
>
> float DBgain =
> theSiPixelGainCalibrationService_->getGain(detid_, col, row);
> - float DBpedestal =
> theSiPixelGainCalibrationService_->getPedestal(detid_, col, row) *
> DBgain;
> + float pedestal =
> theSiPixelGainCalibrationService_->getPedestal(detid_, col, row);
> + float DBpedestal = pedestal * DBgain;
> + //float DBpedestal =
> theSiPixelGainCalibrationService_->getPedestal(detid_, col, row) *
> DBgain;
>
> Please remove unneded commented out code
>
> In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
>
> > @@ -351,6 +358,26 @@ int PixelThresholdClusterizer::calibrate(int
> adc, int col, int row)
> } else {
> electrons = int( vcal * theConversionFactor + theOffset);
> }
> +
> +
> + // if(adc<0 || adc>255) cout<<" adc wrong range "<<adc<<endl;
>
> Please remove unneded commented out code, or add a comment which
> explains why it is needed here
>
> In RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc:
>
> > + // if(adc<0 || adc>255) cout<<" adc wrong range "<<adc<<endl;
> +
> + // if(layer_>0) {
> + // if( (layer_==1 && (pedestal<-40. || pedestal>170.) ) ||
> + // (layer_==2 && (pedestal<-260.|| pedestal>230.) ) ||
> + // (layer_==3 && (pedestal<-130.|| pedestal>80.) ) ||
> + // (layer_==4 && (pedestal<-90. || pedestal>110.) ) )
> +
> + // cout<<"PED outside range: layer "<<layer_<<"
> "<<pedestal<<" "<<DBgain<<" "<<DBpedestal
> + // <<" "<<adc<<" "<<vcal<<" "<<electrons<<" det: "
> + // <<detid_<<" row: "<<row<<" col: "<<col<<endl;
> +
> + // }
> +
> + if(electrons<0)
> + cout<<" layer "<<layer_<<" "<<adc<<" "<<DBgain<<"
> "<<pedestal<<" "<<DBpedestal<<" "<<vcal
>
> cout is not thread safe
> I suspect anyhow that also this line was originally supposed to get
> commented out
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#20555 (comment)
|
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
please test |
|
The tests are being triggered in jenkins. |
|
@dkotlins @makortel @VinInn may be able to help with the tracking validation plots. |
|
Yes, there are differences. This is why this code modification was done. It recovers pixels lost dues to negative charge.
It should improve the reconstruction but some distribution will look different (e.g. cluster charge and size, pixel charge).
… On 18 Sep 2017, at 17:00, perrotta ***@***.***> wrote:
As expected, the jenkins tests show sizeable differences in all reco - related quantities (i.e. tracking anf basically everything higher level).
Comments and justifications are needed before we can continue with the review.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I have gone back with most of the thresholds to their previous values. Only the channel (pixel) threshold is new,
this has to stay (<100) if we want the fix to work.
… On 18 Sep 2017, at 15:39, perrotta ***@***.***> wrote:
(Linked to the comment from @slava77 above). Were the large modifications in the thresholds in SiPixelClusterizer_cfi.py already discussed and approved within the Tracking POG? Can you put a link to a presentation where its effect on tracking quantities is shown?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Slides were presented at the Tracker DPG meeting on Tuesday.
… On 18 Sep 2017, at 14:52, Slava Krutelyov ***@***.***> wrote:
@dkotlins
please add some slides to the PR description which would describe the effect of this PR on physics outputs.
If this affects inputs for tracking in a significant way, some tracking validation plots would be nice as well.
@makortel @VinInn may be able to help with the tracking validation plots.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
This fix does affect reco Tracks and all higher level quantities. There is the request to integrate it in 940pre2, and have therefore it tested with the needed related payloads (see also https://indico.cern.ch/event/666794/contributions/2729097/attachments/1528713/2391465/tanjaReco_22092017.pdf) . Did you already manage to make your validation plots combining this PR with the new payloads, or do you plan to provide some? Could you please comment about the request of integrating this PR in 94X? Thank you. |
No (I don't think we've promised such, and anyway to my understanding the effect on MC should be minimal so MTV wouldn't tell much on the effect on data).
Please integrate. |
|
Thank you Matti.
Matti Kortelainen <notifications@github.com> ha scritto:
@perrotta
> Did you already manage to make your usual multiTrack validation
> plots combining this PR with the new payloads, or do you plan to
> provide some?
No (I don't think we've promised such, and anyway to my
understanding the effect on MC should be minimal so MTV wouldn't
tell much on the effect on data).
Indeed, the reference to MTV was incorrect, as the effect is mostly on data.
And yes, you never promised any validation, as far as I know, although
we repeatedly asked the authors of this PR (even within this github
thread) to get confirmation from the tracking POG that everything was
agreed with you, and to provide some possible validation with your help.
> Could you please comment about the request of integrating this PR in 94X?
Please integrate.
We are near the latest open 94X pre-release: I take this as a blessing
of this PR by the tracking POG.
… --
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#20555 (comment)
|
|
Here are a few validation plots and comparisons from the SinglePh2017B wf 136.788 run by jenkins, to be kept here as a possible low stat reference (only 100 events run by jenkins). The new payloads that must be associated with this PR were not included .
|
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
@perrotta Here is some "post-merge validation" with 2000 events of JetHT run 302131. Full DQM GUI can be found from
With this PR (wrt. gain v5) we get a tiny bit more quadruplets Resolution improves tiny bit BPix1 hit efficiency (from track hit pattern) improves tiny bit In conclusion the changes look small but are in good direction (as expected). Note that to profit maximally (in resolution) we need also new templates and alignment. |
This is a one line code addition to the pixel clusterizer to take care of very low pixel charges.
It has to be accompanied by the lowering of all pixel clusterizer thresholds. Especially the pixel
threshold (called channelThreshold) has to be below 100 in order to recover low charge pixels.