Fix roc width in badComponents method #19881
Conversation
|
A new Pull Request was created by @VinInn (Vincenzo Innocente) for master. It involves the following packages: RecoTracker/MeasurementDet @perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild , please test |
|
The tests are being triggered in jenkins. |
|
it can be considered a bug-fix |
VinInn
changed the title
|
+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:
|
| if(measDet.isActive()){ | ||
| MeasurementDetWithData const & measDet = measTk->idToDet(id); | ||
| if(measDet.isActive() && !measDet.hasBadComponents(detWithState.front().second)){ | ||
| // if(measDet.isActive()){ |
There was a problem hiding this comment.
is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
from the context, it looks like the idea is to allow to flip one way or the other. shouldn't there be a config flag to handle this rather than a hardcoded choice?
There was a problem hiding this comment.
it was already there (the opposite)
I just moved the comments from one line to the other...
not obvious, no reason to be configurable at run time.
In my opinion it was removed because of the bug (a factor 400 in the area covered by the Roc) that was giving too many false positive.
If (mini-aod) users wants count inactive as missing they can just ignore MissingInner all-together...
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
Pull request #19881 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again. |
|
@cmsbuild, please test |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
On 7/27/17 1:12 AM, Vincenzo Innocente wrote:
Pixel is done by hand.
@boudoul @veszpv @jkarancs @tvami to comment further about pixel bad
components
a breakdown between full inactive dets and the badComponents (bad ROCs)
would be nice to know.
Looking at the code and configuration,
apparently what we have now is
- full dets are marked inactive event by event if unpacker detects a ROC
timeout error
- badComponents (bad ROCs) are set by SiPixelQualityFromDbRcd
So, it looks like everything "static" is coming from badComponents and
will be picked up by changes in this PR.
on BPIX1 eff vs phi
which one is "after" here
The flat one
https://user-images.githubusercontent.com/4143702/28660648-1d63b820-72b4-11e7-8643-1a276a248c97.png
or the one with a dip
https://user-images.githubusercontent.com/4143702/28660648-1d63b820-72b4-11e7-8643-1a276a248c97.png
?
Are these eff vs phi for BPIX1 based on effFromHitPattern setup or is
it something else?
Is this a real thing coming through TkPixelMeasurementDet::measurements
(the other part where badComponents are used)
or is it the secondHitPattern-related?
|
|
what comes out of TkPixelMeasurementDet::measurements is irrelevant for BPIX1 as the track (out-in by definition) will stop anyhow and invalid-hits at the extremes of trajectories are evicted asap. the plots comes out of AOD (so HitPattern) for BPIX1 so it is secondHitPattern-related |
this would be a long development and most probably irrelevant to physics (a ROC is big enough w/r/t tracking precision to produce a visible sharp hole) |
|
in any case the info about "BAD ROC" at runtime (event by event) will be soon improved. |
|
On 7/27/17 7:32 AM, Vincenzo Innocente wrote:
|a breakdown between full inactive dets and the badComponents (bad ROCs)
would be nice to know. |
this would be a long development and most probably irrelevant to physics
(a ROC is big enough w/r/t tracking precision to produce a visible sharp
hole)
the scope of my question was to get a rough estimate of where the
assignments to inactive or bad happen.
This does not need developments, just a check of known stats.
If I read the code right, all of the continuously bad parts of the
detector are coming through badComponents.
via SiPixelQualityFromDbRcd
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19881 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbnorB_WbNViJcm7or76Epg3-dJigks5sSJ-UgaJpZM4OhVgb>.
|
| //if(measDet->isActive() && !measDet->hasBadComponents(detWithState.front().second)){ | ||
| if(measDet.isActive()){ | ||
| MeasurementDetWithData const & measDet = measTk->idToDet(id); | ||
| if(measDet.isActive() && !measDet.hasBadComponents(detWithState.front().second)){ | ||
| InvalidTrackingRecHit tmpHit(*detWithState.front().first, outIn ? TrackingRecHit::missing_outer : TrackingRecHit::missing_inner); | ||
| track.appendHitPattern(tmpHit, *ttopo); | ||
| //cout << "WARNING: this hit is marked as lost because the detector was marked as active" << endl; |
There was a problem hiding this comment.
from the discussion so far, it seems to me that we may benefit from having inactive hits added at least to the missing_inner pixel dets.
this way a relevant choice can be made downstream in AOD or miniAOD.
There was a problem hiding this comment.
sorry NO, this is the opposite of the logic used so far: inactive hits shall NOT be counted as missing. This is a corner stone of current classification.
There was a problem hiding this comment.
it's a matter of HitPattern interface: the missingInner counts can stay to return what the intended design is;
I'm asking to add the hits for inactive so that they can be checked with relevant accessors.
this is my understanding as well. As said this is going to change soon. |
|
On 7/27/17 7:42 AM, Vincenzo Innocente wrote:
sorry NO, this is the opposite of the logic used so far: inactive hits
shall NOT be counted as missing. This is a corner stone of current
classification.
however the bad components were never ignored
and the reality is that so far all of the static inactive pixel
subdetectors were counted as missing.
It's hard to call something that has never been used a cornerstone.
|
|
Pixel bad componenents were never really filled consistently ( @veszpv to comment ) This is going to change: inactive rocs will be now marked consistently and we need to avoid to consider them active and therefore assign an inefficiency to them. |
|
On 7/27/17 7:54 AM, Marco Rovere wrote:
Ciao @slava77 <https://github.com/slava77> I'm not sure I understand
your statement. As @VinInn <https://github.com/vininn> already said, an
inactive hit should *not* be mixed with a missing hit, since they convey
a completely different meaning and, I bet, many IDs/analyses will be
fooled if we change this.
the code implementation for this "secondHitPattern" has not used
hasBadComponents since at least 2010
and that's what IDs/analyses have trained for.
I understand the point of missing vs inactive that they should not be
confused,
but that's not the way we treated "static" inactive pixel dets up to now.
|
|
On 27 Jul, 2017, at 4:53 PM, Slava Krutelyov ***@***.***> wrote:
it's a matter of HitPattern interface: the missingInner counts can stay to return what the intended design is;
I'm asking to add the hits for inactive so that they can be checked with relevant accessors.
This requires development and will introduce regression.
|
|
On 27 Jul, 2017, at 5:01 PM, Slava Krutelyov ***@***.***> wrote:
the code implementation for this "secondHitPattern" has not used
hasBadComponents since at least 2010
and that's what IDs/analyses have trained for.
I understand the point of missing vs inactive that they should not be
confused,
but that's not the way we treated "static" inactive pixel dets up to now.
—
sorry there is a missanderstanding
```
if(measDet.isActive()){
```
is there since ever AND inactive modules in pixel has been in the database since ever (often even if only half of it was actually off)
This is what IDs/analyses have seen and will continue to see at a better granularity...
v.
|
|
On 7/27/17 8:05 AM, Vincenzo Innocente wrote:
> On 27 Jul, 2017, at 5:01 PM, Slava Krutelyov ***@***.***> wrote:
>
> the code implementation for this "secondHitPattern" has not used
> hasBadComponents since at least 2010
> and that's what IDs/analyses have trained for.
>
>
> I understand the point of missing vs inactive that they should not be
> confused,
> but that's not the way we treated "static" inactive pixel dets up to now.
> —
>
sorry there is a missanderstanding
```
if(measDet.isActive()){
```
is there since ever AND inactive modules in pixel has been in the
database since ever (often even if only half of it was actually off)
This is what IDs/analyses have seen and will continue to see at a better
granularity...
where in the code is the measDet set to be inactive?
The only place I have found so far is done in
MeasurementTrackerEventProducer
based on flags set in the SiPixelRawToDigi, which correspond only to ROC
timeout errors in unpacking (transient event by event cases).
There is also an option to mask the entirety of pixel dets as inactive
if the siPixelClusters are empty,
but that's separate.
…
v.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19881 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbvtXAyG-8OJQoccYNgEbC6dkSs7Mks5sSKdIgaJpZM4OhVgb>.
|
|
On 27 Jul, 2017, at 5:10 PM, Slava Krutelyov ***@***.***> wrote:
where in the code is the measDet set to be inactive?
|
|
On 7/27/17 8:58 AM, Vincenzo Innocente wrote:
> On 27 Jul, 2017, at 5:10 PM, Slava Krutelyov ***@***.***> wrote:
>
> where in the code is the measDet set to be inactive?
http://cmslxr.fnal.gov/dxr/CMSSW/source/RecoTracker/MeasurementDet/plugins/MeasurementTrackerImpl.cc#424
thank you.
I missed that.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19881 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbgOixKTxuhiLli1a4CWPbMaEN78bks5sSLO4gaJpZM4OhVgb>.
|
|
+1
#19940 is created to possibly add ability to count inactive layers as well. |
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
merge |
and activate again its use in vetoing missing inner/outer...
should be visible in scatter plot of missing inner for recent data