Factorization of Track Iteration selections in PF #12715
Conversation
|
A new Pull Request was created by @bachtis (Michalis Bachtis) for CMSSW_8_0_X. It involves the following packages: RecoParticleFlow/Benchmark @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks. Following commands in first line of a comment are recognized
|
|
@bachtis |
|
@bachtis Looks OK to me for EGM tracking business. I would prefer that you make an enum that defines what the tracking algorithm means, instead of just returning some raw integer value. The fewer magic numbers we have running around in the PF code, the better. |
|
Hi @slava77 . It is not the best that can be done but it is the best we can do. In the past we have seen changes in the tails of the RelVal by such additions. Calorimeter takes over but usually we have a TeV track not linked to anything that makes the tail ..[at least based on what we saw this year] @lgray I thought to have an enum but couldnt make a name for that since it is just a purity category ...So at the end I left it like this |
|
@bachtis Well, I just point out that all these categories have some underlying meaning (high quality track or what have you) and it would be useful to make it clear what that meaning is. For you or I it's fine to leave as is, for other people that look at PF it's just a random number and it would be helpful for understanding to label why you're taking a particular iteration. |
| case reco::TrackBase::lowPtTripletStep: | ||
| case reco::TrackBase::pixelPairStep: | ||
| case reco::TrackBase::jetCoreRegionalStep: | ||
| return 0; |
There was a problem hiding this comment.
do these numbers have any conceptual meaning with some intuitive name?
There was a problem hiding this comment.
Well no.. here we group the iterations to some purity categories that are using to apply Dpt/pt cuts.. so if I did enum it would be cat0,cat1 etc...
There was a problem hiding this comment.
Even just saying "highQuality", "mediumQuality", (since the Dpt/pt cut corresponds to the expected quality of the track), would be useful over cat0,cat1,cat2, or 0,1,2.
It doesn't have to be some deep philosophical concept.
There was a problem hiding this comment.
enums are more tractable
for code maintenance
|
OK. I can try tomorrow |
|
What concerns phase1, I don't think it is good use of time to tune the cuts in PF until the tracking has somewhat stabilized. I.e. I would just add the new iterations such that a quadruplet iteration gets treated as the "corresponding" triplet iteration is, and we check later if that works as it is or if further tuning is needed here. But maybe we could take the PFMET tails into consideration already during the tuning of phase1 tracking. |
|
@bachtis concerning PFDisplacedVertexFinder and BTV POG, there is basically already a vertex finder in the POG capable of doing that (IVF) and studies in the POG to ID Nuc Int (for b-tag rejection). |
|
@arizzi good. This is what I wanted to know . We should definitely switch to IVF |
|
Hi , So Looking at the code again I prefer to stay with an unsigned integer : |
|
The thinning of the categorization is very important and here you've already taken a good step towards making this more clear, you can still compare by > or == in the if statements you just get something more expressive rather than > 3 or == 0. |
|
Well but it is a duplication!..We had it like this already |
|
Well In fact I will go with something totally different ... |
|
How? You already have the mapping of the enums defined in one place and you use them elsewhere. Better yet you can define the various cuts you use centrally in this new file and just have descriptive function names if you want to keep unsigned integers. At least then you are given some context for what the integers are. Still prefer enums. |
|
Ah, our mails crossed, if you have something different in mind give it a try. |
|
Pull request #12715 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again. |
|
OK I prefer it like this |
| std::cout << ic << " " << *(constituents[ic]) << std::endl; | ||
| break; | ||
| } | ||
| unsigned int iter = trackRef->algo()>6 ? 6: trackRef->algo(); |
There was a problem hiding this comment.
is this "reduction" of useful values reasonable or harmless enough?
(track algos were mapped on 8, now only on 6 bins)
Previously values from 0 to 7 had a meaning and 8 was everything else.
Now 0 is useless (invalid iter), 1 to 6 have something meaningful.
There was a problem hiding this comment.
Also, please note that #8315 made an attempt to get rid of magic numbers and less/greater than comparisons of TrackBase::algo() (although https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc#L1409 got omitted and should be fixed; I hope in this PR if possible). I would prefer that the new code would not introduce new uses of either.
I have also a more practical question. The numerical value 6 corresponds to pixelPairStep. What should be done with phase1 highPtTripletStep (numerical value 22)? I would put it (as a reasonable first guess) along initialStep (4), but this code maps it to 6.
There was a problem hiding this comment.
Well this is not running in RECO . It is just the histogram that is filled. I can look at it again. IN fact we should purge this package at some point
There was a problem hiding this comment.
Ok. Is it also not run in DQM or VALIDATION (i.e. does it show up in PR tests or RelVals)? If it is not run in the standard sequences, then I could live with it as it is (assuming I don't have to care about it for Phase1 and beyond).
There was a problem hiding this comment.
Hi Matti,
This one I had not seen . In any case I cannot do anything about that since it is EGM code and they should take care of it [In fact I dont think it is running any more either this is why I wanted to have a clean up ]
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Pull request #12715 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
cmsbuild
added
reconstruction-approved
fully-signed
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar |
|
+1 |
Factorization of Track Iteration selections in PF
|
Now that we have an IB with this PR in, when could we expect the "EGM PR"? |
|
Hi, Regards, +++++++++++ On Mon, Dec 14, 2015 at 4:16 PM, Matti Kortelainen <notifications@github.com
|
|
@matteosan1, |
|
@matteosan1 I let Slave to comment what he wants fixed in subsequent PRs, but I'm referring to this
(if we (TRK) are expected to migrate the iteration check for phase1 iterations) |
|
@matteosan1 |
"Almost" technical PR that defines a common place of the treatment of the track iterations in Particle Flow
Almost -> the PFDisplacedVertexFinder seems to be very out-dated wrt the iterations added therefore
I updated it . In the future we should assign this module to a POG (probably BTV)
@lgray @matteosan1 please have a look at the EGM modules . I think I changed them correctly
One other EGM action item is to clean up the PF EGM code that we dont use . I think 80X is a good place to do this so please make sure somebody does it at some point
@VinInn @makortel @rovere you can start from here to add new iterations
Major issue for tracking: You can-NOT just add iterations in the file as it was done in #12577
All the MET tails we had in 2015 was due to tracking so if one iteration introduces MET tails it needs to have some Dpt/pt cuts . So for every PR that has new iterations one should look at the PFMET distribution before and after in RelVal FlatQCD and even one additional event in the tail is enough to tell you there are problems.
So looking forward to see this in the future
@arizzi @gpetruc FYI