Fix phase2 pixel in GeometrySearch #16470
Conversation
Temporary solution to partly fix the Navigation. The problem is the "subDetector" method which should differciate the Pixel Layers with OT ones. This is a dirty solution, but it is working. Better to think about templating the class..
The last ring of the new layout for phase2 pixel extend in r over the min r of the OT. This brings a bug in the navigation that has been fixed.
The Phase2PixelEndcap is not needed anymore, only the classes Phase2EndcapLayer and Phase2EndcapRing are used for both Pixel and OT detector. The names have been changed accordingly.
|
A new Pull Request was created by @ebrondol for CMSSW_8_1_X. It involves the following packages: RecoTracker/TkDetLayers @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@ebrondol |
|
@slava77 yes, in this particular plot I have used the 4 muons wf with pt between 1 and 200 GeV (extended). I reproduced the same behavior also with single mu pt 10 GeV. |
|
ah, I worked on top of the PR #16407, so I had all of them automatically. |
|
Comparison job queued. |
|
Comparison job queued. |
|
@venturia I think that the main changes on performance of the filter is when it is wrong. This is why I wanted to double check that the performance were as expected. When it is off (as in the case of the baseline) or when it has the right correction (as in the case of this PR), this does not seem to affect the tracking performance - as far as I can tell. The problem we found out in the case of filter off, stands in the CPU timing. |
|
On 11/17/16 1:17 AM, ebrondol wrote:
This shows almost no effect on eff or fakes
|
| @@ -11,6 +11,5 @@ | |||
| ) | |||
| from Configuration.Eras.Modifier_phase2_tracker_cff import phase2_tracker | |||
| phase2_tracker.toModify(ClusterShapeHitFilterESProducer, | |||
| PixelShapeFile = 'RecoPixelVertexing/PixelLowPtUtilities/data/pixelShape_Phase1Tk.par', | |||
| doPixelShapeCut = cms.bool(False) | |||
| PixelShapeFile = 'RecoPixelVertexing/PixelLowPtUtilities/data/pixelShape_Phase2Tk.par', | |||
There was a problem hiding this comment.
is this applicable to T1,T2 geometries?
I thought that the pixelShape_Phase1Tk.par was supposed to be applied to T1,T2 (phase-1 like pixel).
Most of the workflows that we have are still T1. It would be bad if behavior there goes bad with pixelShape_Phase2Tk.
There was a problem hiding this comment.
we need T*-specific eras for tracking
There was a problem hiding this comment.
I think the simplest is to have T1+T2 to have different tracker-era than T3.
There was a problem hiding this comment.
Let me remind that the most correct solution would be to move the whole calibration to GT (which would have saved some hairs with phase1 in pre12), but probably we are, again, after a "quicker" solution.
There was a problem hiding this comment.
What if we check before the performance of T1 and T2 with this "wrong" calibration? In any case also the "phase1" ones were not optimal. I know that they are not the right ones for them, but if T3 is the only that we care for TDRs, and the performance on T1 and T2 are also kind of ok with the phase2Par, we do not need to create other eras or so which takes time and maybe confusion later.
I have started to produce results for ttbar in D1 geometry, with and without this PR. It does seem to me evetything all right.. @slava77 what do you think?
There was a problem hiding this comment.
And the result basically means: no so bad if we use phase2 shape filter for phase1, while a disaster if we do the contrary.. interesting.
There was a problem hiding this comment.
The addition of multiple phase2_tracker Eras is possible, but introduces a lot of complexity. If @ebrondol can show the performance is not too bad, it would be easier just to use the same pixel shape everywhere. (@makortel: moving to GT would imply we'd have to create a separate GT queue for each Phase2 detector version... also not ideal.)
There was a problem hiding this comment.
what are the results with PU?
Unless we move all "standard" workflows to use T3 (to include variations with I1/I2, M1/M2, timing or no timing), we should first confirm that these phase2 shapes are ok on T1 in the full dyn range (well, if it helps to speedup the tests, PU140 may be enough)
There was a problem hiding this comment.
as I pointed in a separate email, can we look at comparisons of selections in phase1 and phase2 files compared to how the clusters look like (for good and bad clusters)?
So far the discussion treats the file and overall tracking as black boxes and we just look at high level outputs.
I'll start the job soon. there were actually improvements in a fraction of iterations, but these were balanced out by increases in some others, and most notably in allConversions and ecalDrivenElectronSeeds |
|
@slava77 yes, this is ttbar from 21224 with no PU. |
|
We should have different files for T1-T2 and T3. IS it possible to have different eras? Il giorno 17/nov/2016, alle ore 15:05, ebrondol notifications@github.com ha scritto:
|
|
Some plots with the older version (as of 54052f2 ), these should show the effect of the geometry search alone ttbar PU200 (derived from 21224) the largest change in tracking steps appears to be in pixelPair building chi2 and nhits distribution look in agreement with plots posted with the PR itself. this should be useful for incremental comparison. Since the pixel shape selection is a more significant feature. |
|
is this PR needed in 81X? we can just continue in 90X. the fixes are rather minor in terms of the impact (jobs run, efficiency and fake rate is about the same; timing is not reduced by much, just under 20% in D4 PU200 tests). we can continue discussion in this thread though for simplicity |
|
Hi @slava77 , I am not sure which release will be used for the final production of the TDR. This PR must be for sure in that release. |
|
On 11/20/16 5:37 AM, ebrondol wrote:
It will be based on 90X.
|
|
Here are some notes on the impact of the last changes (54052f2...73649c3 which is due to the pixel shape selections). Based on tests in CMSSW_8_1_X_2016-11-15-2300 for D4 ttbar with PU200. I was looking for low-level plots like OffTrack or OnTrack, but apparently they are not in the phase-2 DQM. These would've been useful. The main impact is on the technical side: CPU decreases by about 15% for the job and by a much more significant fraction for the affected modules (comparing to the IB here): Note that despite a factor of 30 change in CPU in pixelTracks, the final number of pixelTracks reconstructed is about the same. Incremental differences of tracking performance (54052f2..73649c3) show fairly small impact on high level values (eff and fake rate) as expected, there is a slight decrease in the number of hits there is some reshuffle in the final track algos ( more tracks in lowPtTriplet) lowPtTriplet has less seeds Some inclusive statistics:
|
|
+1
|
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
+1 |
This PR has been developed in CMSSW_8_1_X_2016-11-01-2300 and tested on top of the PR #16407. This is the evolution of the story: with a private branch that produce the hit on track maps for phase2 geometry, I produced the ratio clusters/clusters on track in D4:
It is pretty clear that the forward phase2 pixel detector is missing some hits and the last rings of the last forward layers[8-11] are not included in the search.
The problem has been cured using not anymore the class
PixelForwardLayerBuilderbut instead adaptingPhase2OTEndcapLayerBuilderto be also used for the forward phase2 pixel. In this respect the naming has been changed everywhere.After this fix, the new map for D4 looks like:
The code has been changed in such a way that the other D* should not be affected.
This impact some track variables such as the #hits vs eta, just two examples for single mu and for ttbar. But both seem now much more reasonable respect to before. If you want to have a look at the full set of results you can find them here and there. No huge differences have been noticed in eff/fakes.
The documentation in
RecoTracker/TkDetLayers/doc/TkDetLayersTree.pdfhas also been update - including the tilted update that was never introduced.Thanks to @VinInn @rovere @venturia
Informing @delaere @boudoul @atricomi @kpedro88