Stabilize deltaPhi

[VinInn]

implement deltaPhi w/o while loop (required among others by Ofast to avoid infinite loops due to NaN)
took longer because
Geom::Phi (that I will evict from CMSSW at some point) is incompatible with the new implementation
HI compiles some CGAL stuff with rounding-math that is incompatible with the use of compile-time expressions
deltaPhi-like algorithms exist in many places including two identical implementation of normalizedPhi
fixed also in TripletSeed and the origin of nan in tkdetUtil

needless to say that it will recompile everything
expect regression due to possible small numerical differences

[cmsbuild]

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

It involves the following packages:

Alignment/OfflineValidation
Alignment/TrackerAlignment
Calibration/EcalAlCaRecoProducers
CommonTools/Utils
DataFormats/GeometryVector
DataFormats/Math
EventFilter/SiPixelRawToDigi
MuonAnalysis/MuonAssociators
RecoHI/HiJetAlgos
RecoMET/METAlgorithms
RecoMuon/GlobalTrackingTools
RecoMuon/MuonIdentification
RecoMuon/MuonIsolation
RecoMuon/MuonSeedGenerator
RecoTracker/ConversionSeedGenerators

@civanch, @diguida, @cvuosalo, @cerminar, @monttj, @cmsbuild, @franzoni, @mdhildreth, @slava77, @vadler, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @TaiSakuma, @yslai, @abbiendi, @argiro, @Martin-Grunewald, @tlampen, @dgulhan, @mmarionncern, @battibass, @makortel, @jhgoh, @amagitte, @jdolen, @yenjie, @cerati, @trocino, @rociovilar, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @nhanvtran, @tocheng, @schoef, @mschrode, @mmusich, @richard-cms, @mandrenguyen, @jazzitup, @ahinzmann, @dkotlins, @kurtejung, @gpetruc, @istaslis, @mariadalfonso, @yetkinyilmaz, @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/10377/console

[davidlt]

You can force compiler to produce "compatible" results between run-time and compile-time, but that mostly means utilising less efficient instructions (e.g. it could disable FMA).

[cmsbuild]

Pull request #12843 was updated. @civanch, @diguida, @cvuosalo, @cerminar, @monttj, @cmsbuild, @franzoni, @mdhildreth, @slava77, @vadler, @mmusich, @davidlange6 can you please check and sign again.

[VinInn]

@cmsbuild , please test

[cmsbuild]

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

[cmsbuild]

-1
Tested at: bb11ab4
I found an error when building:

>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/HiGenCleaner.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/MultipleAlgoIterator.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ParametrizedSubtractor.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ParticleTowerProducer.cc 
In file included from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ParticleTowerProducer.cc:35:0:
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/poison/RecoHI/HiJetAlgos/plugins/ParticleTowerProducer.h:1:2: error: #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
 #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
  ^
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ReflectedIterator.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/UEParameters.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/VoronoiSubtractor.cc 


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12843/10377/summary.html

[cmsbuild]

-1
Tested at: 5b47d9b
I found an error when building:

>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/MultipleAlgoIterator.cc 
Copying tmp/slc6_amd64_gcc493/src/CondFormats/SiPixelObjects/test/testSerializationSiPixelObjects/testSerializationSiPixelObjects to productstore area:
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ParametrizedSubtractor.cc 
Leaving library rule at src/CondFormats/SiPixelObjects/test
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ParticleTowerProducer.cc 
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ParticleTowerProducer.cc:35:63: fatal error: RecoHI/HiJetAlgos/interface/ParticleTowerProducer.h: No such file or directory
 #include "RecoHI/HiJetAlgos/interface/ParticleTowerProducer.h"
                                                               ^
compilation terminated.
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-22-1100/src/RecoHI/HiJetAlgos/src/ParticleTowerProducer.cc:35:63: fatal error: RecoHI/HiJetAlgos/interface/ParticleTowerProducer.h: No such file or directory
 #include "RecoHI/HiJetAlgos/interface/ParticleTowerProducer.h"


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12843/10378/summary.html

[slava77]

I'm not sure why we should leave a broken implementation of acos.
acos(-1) should be pi and it is not much more complex than the current implementation.
Otherwise the function should probably have an assert for negative argument values.

[VinInn]

ok, I already made the change will copy and commit here...

[VinInn]

@cmsbuild, please test

[cmsbuild]

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

[cmsbuild]

Pull request #12843 was updated. @civanch, @diguida, @cvuosalo, @cerminar, @monttj, @cmsbuild, @franzoni, @mdhildreth, @slava77, @vadler, @mmusich, @davidlange6 can you please check and sign again.

[cmsbuild]

+1
Tested at: 9777bb0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12843/10395/summary.html

[cmsbuild]

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

[VinInn]

There are no regression.
I suggest to merge this PR a.s.a.p. to progress with gcc5.3 validation and allow TRK-POG to continue development: I have already some branches that will not merge anymore.

[VinInn]

btw I made a quick study of the origin of large Phi-windows...
It comes as no surprise that most of then happen in convTrackCandidates CkfTrackCandidateMaker
(yes, we need a serious study, validation and tuning of conversions!)
The rest, besides being an order of magnitude less, are typically low-pt grazing trajectories (often just one track producing many incidents).
Sincerely I am a bit dubious about the validity/utility of a trajectory with local errors in excess of 10cm...

[slava77]

there are some regressions in my local tests.
I'll see if any are significant later today and if they are triggered by something else in this PR.

[slava77]

+1

for #12843 9777bb0

• code changes are in line with the description
• jenkins tests pass and show single-track difference in 135.4 in fwlite comparisons
• more tests were done locally: there are fairly rare or small differences triggered either by numerical difference in the implementation of deltaPhi or in degrees or from (more likely source?) clamping of the acos argument in tkDetUtil::computeWindowSize.
  • Single-track differences are present (somehow mostly in run2 workflows) in about 1 out 10K tracks or less; DQM plots show that there are maybe x2 more starting from seeding (mixedTriplet and detachedTriplet were most frequent) and decreasing to occasionally no effect at the final track level
  • tof_phi histogram from TrackerHitsV (plots gpos.phi().degrees()) has differences at ~2E-5 rate, which looks like floating point precision
  • seedsOfDisplacedSTAMuons have numerical level diffs as well

[civanch]

+1
@VinInn , @slava77 , we had a problem of the same kind as wrong acos in 2015, when GeantV team reported about wrong CMS geometry in GDML dump. The origin of the problem was 'numerical receipt' - we spent a lot of time before understand that this receipt in some specific case fails. So, any fast math or other short cut in mathematics needs full validation in all possible conditions.