add a specific duplicateMerge algorithm

[VinInn]

duplicate-Merged tracks were inheriting the algorithm of the innermost piece.
This was causing confusion at many levels (besides the difficulties in validating and analyzing these tracks)

Here we assign them their own algo flag.

purely technical change. Track migration form previous algo to the new one expected.
see https://innocent.web.cern.ch/innocent/RelVal/pu15_80_dma/
If physics change, there is a bug downstream, most probably in code other than tracker.

[cmsbuild]

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_8_0_X.

It involves the following packages:

DataFormats/TrackReco
RecoParticleFlow/PFProducer
RecoParticleFlow/PFTracking
RecoTracker/FinalTrackSelectors

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @mmarionncern, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @mschrode, @cerati, @gpetruc, @istaslis, @lgray, @dgulhan, @bachtis this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

• +1|approve[d]|sign[ed]: L1/L2's to approve it
• -1|reject[ed]: L1/L2's to reject it
• assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
• unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
• hold: L1/all L2's/release manager to mark it as on hold
• unhold: L1/user who put this PR on hold
• merge: L1/release managers to merge this request
• [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
• [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

[VinInn]

@cmsbuild, please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9961/console

[VinInn]

@makortel, TMV will need an update

[cmsbuild]

Pull request #12577 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

[VinInn]

@cmsbuild, please test

[makortel]

Do you want to pick MTV update here or shall I do a separate PR?

Btw, shouldn't this
https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/DataFormats/TrackReco/src/TrackBase.cc#L12
be changed too?

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9963/console

[VinInn]

@makortel , yes forgot L12... wil fix here
I think MTV update better in a separate PR... (hope TrackBase.cc#L12 is last place rs is quoted...)
I am afraid more places that select 4<algo<12 or similar....
in any case your new phaseI algo may be affected by this type of code...

[cmsbuild]

Pull request #12577 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

[makortel]

There shouldn't be any relative comparisons of algos left after #8315 (unless something has creeped in).

[VinInn]

@cmsbuild, please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9966/console

[makortel]

@VinInn Noted, although with Phase1 we have slightly (but only slightly) different situation, because the behaviour in PF (and others) got changed already by #5807 (something I learned only last week and wish would have realized earlier...), and thus "the current behaviour" in PF for Phase1 is probably already not what we want.

[makortel]

Regarding my earlier comment #12577 (comment), I stand corrected. There are still less/greater than comparisons of the algo enumerators (e.g. here
https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc#L1409
) that I did not fix in #8315 (maybe I missed them, maybe I had a reason, can't remember).

Before making use of the #12458-introduced Phase1 enumerators (that's how I encountered the case above), maybe it would be best to make a PR replacing the remaining less/greater than comparisons with explicit lists of enumerators (I won't create a PR before this one gets merged to avoid conflicts)? Or are there better suggestions?

[VinInn]

@makortel , and all
I think we should meet with PF and find out a way to factorize out all those algo based selections
and eventually substitute them with a more physics-based quality criteria
We cannot continue to edit the very same codelet in a gazzillion+1 files
(phaseII integration is foreseen in the very same timescale...)

[makortel]

@VinInn, agreed, that would indeed help (so I will not start yet #12577 (comment))

[cmsbuild]

+1
Tested at: 259fd1a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12577/10062/summary.html

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12577/10062/summary.html

[VinInn]

admittingly this time regressions are absent...

[slava77]

No regressions are much better and easier.
Also, good to know that they have been apparently triggered by missing fixes.

[slava77]

well, it's a "less changes", actually. Not "none at all"

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-12-01-2300+12577/11630/validateJR/all_OldVSNew_RunMinBias2011Awf1000p0/

there are many more in the DQM plots including MTV plots for /general tracks
e.g. 25202
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-12-01-2300+12577/11630/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/Tracking_TrackFromPV_generalFromPV_trackingParticleRecoAsssociationSignal.html

the less selective /general MTV plots are cleaner
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-12-01-2300+12577/11630/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/Tracking_Track_general_trackingParticleRecoAsssociation.html

while the generalProperties plots are unchanged as seen from fwlite plots as well
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-12-01-2300+12577/11630/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/Tracking_TrackParameters_generalTracks_GeneralProperties.html

I was expecting that the generalTracks validation would be unchanged

[VinInn]

FromPV... @makortel any idea/comment?

[makortel]

The FromPV ones (@slava77's second link) are all but one the _Mean or _Sigma that are produced by fits in DQMGenericClient (that to my understanding can be caused by ~any difference in earlier fits). The one exception is h_algo, where differences are expected. The situation is actually the same in the full generalTracks (third link).

[slava77]

On 12/2/15 12:10 PM, Matti Kortelainen wrote:

The FromPV ones (@slava77 https://github.com/slava77's second link)
are all but one the |_Mean| or |_Sigma| that are produced by fits in
DQMGenericClient (that to my understanding can be caused by ~any
difference in earlier fits). The one exception is |h_algo|, where
differences are expected. The situation is actually the same in the full
generalTracks (third link).

Are the tracks filled somehow in a different order?
This would explain differences in fits and numerical precision level
changes in the histograms.

—
Reply to this email directly or view it on GitHub
#12577 (comment).

[slava77]

+1

for #12577 259fd1a

• changes in the code are in line with the description. Since the changes introduce a new high quality type iteration to which a fraction of tracks migrate, there can still be observable impact for the code relying on specific list of iterations at analysis level or similar.
• jenkins tests pass
  • in fwlite comparisons, of all workflows, only in one (1000.0) there is one charged hadron (13342 -> 13343) extra. A small change can be expected due to reassignment sometimes of a track from pixelless or tobtec after a merge to become a duplicateMerge: this will trigger different selection logic.
  • in DQM, workflows without changes in fwlite show a large number of discrepancies, which can be categorized and explained as follows
    • algo or algo-driven plot change
    • fits (plot post-processing) to track or vertex parameter plots have differences at numerical precision level. This can happen for Minuit due to changes in fit results of previously fit plots with actual differences and some memory of the past fit state.

[cmsbuild]

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

[davidlange6]

+1