Fix phi windows computation #12882

VinInn · 2016-01-06T18:11:12Z

This PR is a continuation of my two last ones.

fix phi window computation in seeding: it also adds a maximal phi window mostly to avoid crazy seeds that may eventually generate Nans
Introduce a MaximalDisplacement in Chi2Estimator again to avoid crazy trajectory and the generation of Nans: Conversions should be unaffected
fix long standing bug in computeWindowSize (was not retiring a negative window when required)

took the opportunity for some minor cleanup
A Major cleanup and refactoring of the three major seeding routines is badly needed.
(may occur in time for 8_0_0 if resources available)

Minor regression, timing improvement observed
MTV for the usual 1000 ttbar events PU35 at
http://innocent.home.cern.ch/innocent/RelVal/pu35_80_Xwin21/
(compared to current IB)

cmsbuild · 2016-01-06T18:11:32Z

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

It involves the following packages:

DataFormats/Math
RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaPhotonProducers
RecoPixelVertexing/PixelTriplets
RecoTracker/ConversionSeedGenerators
RecoTracker/MeasurementDet
RecoTracker/TkDetLayers
RecoTracker/TkMSParametrization
RecoTracker/TkSeedGenerator
RecoTracker/TrackProducer
TrackingTools/GsfTracking
TrackingTools/KalmanUpdators

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @Sam-Harper, @battibass, @makortel, @abbiendi, @GiacomoSguazzoni, @jhgoh, @lgray, @bellan, @mschrode, @rovere, @cerati, @gpetruc, @istaslis, @VinInn, @trocino, @dgulhan 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 · 2016-01-06T18:12:07Z

@cmsbuild, please test

cmsbuild · 2016-01-06T18:12:23Z

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

davidlt · 2016-01-06T18:22:55Z

@VinInn does this solves the issue with infinite loop due to NaNs in 10039.0 TTbar_14TeV on GCC 5.3.0?

VinInn · 2016-01-06T18:24:44Z

@davidlt , should, how many events were needed to trigger the infinite loop? (or is enough 10039.0 out of the box?)

davidlt · 2016-01-06T18:27:07Z

out-of-the-box is enough and it hangs on 7th event (quite fast after launching the workflow)

VinInn · 2016-01-06T18:30:02Z

Done                          runTheMatrix.py --useInput=all --list=10039.0 
10039.0_TTbar_14TeV+TTbar_Tauola_14TeV_2017_GenSimFullINPUT+DigiFull_2017+RecoFull_2017 Step0-PASSED Step1-PASSED Step2-PASSED  - time date Wed Jan  6 19:29:10 2016-date Wed Jan  6 19:26:30 2016; exit: 0 0 0
1 1 1 tests passed, 0 0 0 failed

slava77 · 2016-01-06T19:05:32Z

RecoTracker/TkDetLayers/src/TkDetUtil.cc

@@ -43,6 +43,9 @@ namespace tkDetUtil {
      auto sp = (x0*x1+y0*y1)/std::sqrt((x0*x0+y0*y0)*(x1*x1+y1*y1));
      sp = std::min(std::max(sp,-1.f),1.f);
      dphi = std::acos(sp);      
+
+      // if (dphi>0.5*M_PI) std::cout << "large dphi " << dphi << ' ' << ts.localDirection() << ' ' << ts.globalMomentum().perp() << ' ' << maxDistance << std::endl;


is this important to have here?

nope. was planning to add a max-dphi limit: we need conversion to be under control first though.
Will remove

cmsbuild · 2016-01-06T20:19:49Z

+1
Tested at: bfd721f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12882/10435/summary.html

VinInn · 2016-01-08T09:13:46Z

Hardwired default moved to max-float.
This should affect only muons and gsf (tracker uses python default)

took the opportunity to fix a logic error unveiled by gcc530 in DQM

cmsbuild · 2016-01-08T11:26:27Z

+1
Tested at: 03b9f3c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12882/10443/summary.html

cmsbuild · 2016-01-08T12:25:10Z

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

slava77 · 2016-01-08T13:09:18Z

CalibTracker/SiStripHitEfficiency/src/HitEff.cc

@@ -408,7 +408,7 @@ void HitEff::analyze(const edm::Event& e, const edm::EventSetup& es){
 	  const TrajectoryStateOnSurface tsosTEC9 = itm->updatedState();

 	  // check if track on positive or negative z
-	  if (!iidd ==  StripSubdetector::TEC) cout << "there is a problem with TEC 9 extrapolation" << endl;
+	  if (!(iidd ==  StripSubdetector::TEC)) cout << "there is a problem with TEC 9 extrapolation" << endl;


this appears to be the only "bare" cout (not protected by DEBUG).
The bug disabled it.
If it should stay exposed for default processing, it has to be converted to LogWarning/LogInfo.
Otherwise, please add "& DEBUG"

is not under my direct responsibility...
I will protect with DEBUG...

cmsbuild · 2016-01-08T14:25:15Z

Pull request #12882 was updated. @diguida, @cvuosalo, @cerminar, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please check and sign again.

VinInn · 2016-01-08T14:28:13Z

@cmsbuild, please test

cmsbuild · 2016-01-08T14:28:34Z

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

cmsbuild · 2016-01-08T16:39:40Z

+1
Tested at: 66c7f9e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12882/10448/summary.html

cmsbuild · 2016-01-08T17:25:44Z

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

slava77 · 2016-01-08T21:24:45Z

+1

for #12882 66c7f9e

code changes are ok; the last updates fixed issues with the cosmic muons
jenkins tests pass
tested 03b9f3c in CMSSW_8_0_X_2016-01-05-2300 locally with more samples and compared to bfd721f (see summary for bfd721f above in Fix phi windows computation #12882 (comment)). The following is a summary of the effect of going back to float ::max for the max distance.
- in fwlite comparisons: the only changes are in CSCLooseHaloId and in muonsFromCosmics* plots, which now go back to what was there in the baseline.
- in DQM comparisons:
  - there are also very small differences in displaced*Muons plots (apparently going back to what was there in the baseline, confirmed only on a few plots);
  - several workflows have ~0.01 less off-track strip clusters in HLT monitoring plots and no matching increase in on-track plots (this effect looks too small to need a follow up).

Fix phi windows computation

VinInn added 12 commits December 27, 2015 12:40

fix acos

1c27098

fix phi range computation, reduce max window

59a6384

comment cout

7fa0e99

fix windows in seeding (part1)

9ff211f

merge

602a252

factorize, fix LTip as well

87ee93f

fix byChi2 as well

f7c52b9

fix bug and add limit on phiwindow

21044aa

simplify phiRange

3ee5181

merge head

1e7d0a9

Merged PhiWindows from repository VinInn

3a362fa

clean up

bfd721f

cmsbuild added this to the Next CMSSW_8_0_X milestone Jan 6, 2016

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Jan 6, 2016

cmsbuild added tests-started and removed tests-pending labels Jan 6, 2016

slava77 reviewed Jan 6, 2016
View reviewed changes

cmsbuild removed the tests-started label Jan 6, 2016

cmsbuild added the tests-started label Jan 8, 2016

cmsbuild added tests-approved and removed tests-started labels Jan 8, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jan 8, 2016

slava77 reviewed Jan 8, 2016
View reviewed changes

protect cout with DEBUG flag

66c7f9e

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-approved labels Jan 8, 2016

cmsbuild added tests-started and removed tests-pending labels Jan 8, 2016

cmsbuild added tests-approved and removed tests-started labels Jan 8, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jan 8, 2016

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jan 8, 2016

davidlange6 added a commit that referenced this pull request Jan 9, 2016

Merge pull request #12882 from VinInn/PhiWindows

0440f86

Fix phi windows computation

davidlange6 merged commit 0440f86 into cms-sw:CMSSW_8_0_X Jan 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix phi windows computation #12882

Fix phi windows computation #12882

VinInn commented Jan 6, 2016

cmsbuild commented Jan 6, 2016

VinInn commented Jan 6, 2016

cmsbuild commented Jan 6, 2016

davidlt commented Jan 6, 2016

VinInn commented Jan 6, 2016

davidlt commented Jan 6, 2016

VinInn commented Jan 6, 2016

slava77 Jan 6, 2016

VinInn Jan 6, 2016

cmsbuild commented Jan 6, 2016

VinInn commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

slava77 Jan 8, 2016

VinInn Jan 8, 2016

cmsbuild commented Jan 8, 2016

VinInn commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

slava77 commented Jan 8, 2016

Fix phi windows computation #12882

Fix phi windows computation #12882

Conversation

VinInn commented Jan 6, 2016

cmsbuild commented Jan 6, 2016

VinInn commented Jan 6, 2016

cmsbuild commented Jan 6, 2016

davidlt commented Jan 6, 2016

VinInn commented Jan 6, 2016

davidlt commented Jan 6, 2016

VinInn commented Jan 6, 2016

slava77 Jan 6, 2016

Choose a reason for hiding this comment

VinInn Jan 6, 2016

Choose a reason for hiding this comment

cmsbuild commented Jan 6, 2016

VinInn commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

slava77 Jan 8, 2016

Choose a reason for hiding this comment

VinInn Jan 8, 2016

Choose a reason for hiding this comment

cmsbuild commented Jan 8, 2016

VinInn commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

cmsbuild commented Jan 8, 2016

slava77 commented Jan 8, 2016