Introduction of Lorentz Angle correction in Phase 2 CPE #16392

lowette · 2016-10-30T23:27:15Z

This PR is made using @ebrondol 's developments in ebrondol:phase2_movingToCPEwithLA, with on top the inclusion in the Phase2 OT CPE of Lorentz Angle corrections.
Validation of the LA correction was shown in the Phase2 simulation meeting on 28 Oct.
Notifying also @delaere @boudoul

cmsbuild · 2016-10-30T23:27:33Z

A new Pull Request was created by @lowette (Steven Lowette) for CMSSW_8_1_X.

It involves the following packages:

RecoLocalTracker/Phase2TrackerRecHits
RecoTracker/MeasurementDet
RecoTracker/TransientTrackingRecHit

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

cms-bot commands are listed here #13028

slava77 · 2016-10-30T23:50:47Z

RecoLocalTracker/Phase2TrackerRecHits/plugins/Phase2TrackerRecHits.cc

@@ -77,11 +77,11 @@ void Phase2TrackerRecHits::produce(edm::StreamID sid, edm::Event& event, const e
    // Container for the clusters that will be produced for this modules
    Phase2TrackerRecHit1DCollectionNew::FastFiller rechits(*outputRecHits, DSViter.detId());

-    for (auto clustIt : DSViter) { 
-      ClusterParameterEstimator< Phase2TrackerCluster1D >::LocalValues lv = cpe->localParameters(clustIt, *geomDetUnit);
+    for (edmNew::DetSet<Phase2TrackerCluster1D>::const_iterator clustIt = DSViter.begin(); clustIt != DSViter.end(); ++clustIt) { 


was this change from auto and range loop really necessary?

If not the cluster link wasn't stored properly, the Ref below would point to something random (and cause segfaults if accessed). Maybe there are better ways to do this, but the "auto clustIt" out of the box, with then "&clustIt" when making the Ref just doesn't work.

Maybe the range loop should have const auto& (i.e. reference instead of copy)?

(same comment for auto DSViter on line 70 above)

@lowette: How does this version of the code work? Doesn't DSViter get a copy of each element of *clusters? Then how does makeRefTo in line 84 make sense of comparing the vector of Phase2TrackerCluster1D in clusters to the copy of that vector in DSViter?

It would seem safer to make line 70:

for (const auto &DSViter : *clusters) {

and line 80:

for (const auto &clustIt : DSViter) {

so there's no copying, and then &clustIt would point back into clusters, so the original versions of line 81 and 84 could be used.
In this case, the names of DSViter and clustIt should be changed, because neither of them are iterators. DSViter could be called clusterDetSet, and clustIt could be called clusterRef.

Ok, this works, though clusterRef is maybe confusing too, since it is a concrete type (if I got it correctly; "auto" was introduced after I switched to <5% coding).
I hope there was some advantage of changing this, so that I (and you) did not waste time changing something that was working anyway.

slava77 · 2016-10-30T23:51:43Z

@cmsbuild please test

cmsbuild · 2016-10-30T23:51:55Z

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

cmsbuild · 2016-10-31T01:25:33Z

+1
Tested at: 11f38d1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16069/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
5c21004
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16069/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16069/git-merge-result

cmsbuild · 2016-10-31T01:25:36Z

Comparison job queued.

cmsbuild · 2016-10-31T02:30:27Z

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

boudoul · 2016-11-01T08:52:42Z

RecoLocalTracker/Phase2TrackerRecHits/python/Phase2StripCPEESProducer_cfi.py

-  parameters    = cms.PSet()
+  parameters    = cms.PSet(
+    LorentzAngle_DB = cms.bool(False),			
+    TanLorentzAnglePerTesla = cms.double(0.07)


shouldn't it be 0.07 for pixel and 0.106 for strips ? it is at least how it is defined in the digitzer https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SimTracker/SiPhase2Digitizer/python/phase2TrackerDigitizer_cfi.py

The pixel LA is 0.106 [https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SimTracker/SiPhase2Digitizer/python/phase2TrackerDigitizer_cfi.py#L43], while long pixel [https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SimTracker/SiPhase2Digitizer/python/phase2TrackerDigitizer_cfi.py#L79], strip in PS [https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SimTracker/SiPhase2Digitizer/python/phase2TrackerDigitizer_cfi.py#L115], and strips in 2S [https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SimTracker/SiPhase2Digitizer/python/phase2TrackerDigitizer_cfi.py#L151] are all 0.07.

ok thanks , but then I see that here the Strip CPE is defined in this PR , what is the status/plans for Pixel CPE in PS ?

ok maybe I'm confused and you are treating both cases (2S and PS ) modules here ? [I'm maybe fooled by names like Phase2StripCPE which could actually be phase2OTCPE ] ?

Yes, both P in PS, S in PS, and S in 2S are covered.
True, phase2OTCPE would be much better. Several of the file names in that package are not well chosen... If the powers that be prefer, this can of course be changed, but maybe now is not the best moment.

Yes, I agree with @lowette. Changing names can propagate problems in different part of the code. In this moment we have just one CPE for both with a name that is for sure nor descriptive either optimal, but I would suggest to change it when we will have both CPE for P and S separately..

boudoul · 2016-11-01T11:11:50Z

RecoLocalTracker/Phase2TrackerRecHits/test/RecHitsValidation.cc

+          return (100 + topo->tecRing(detid));
+        } else {
+	  return 999;
+	}


Can you Remind me why we need this TID, TOB TIB etc.. structure in this phase2 file ?

In Phase2, such as in Run2 and Phase1, the Pixel/StripSubdetectors name are still the same. In this moment, for Phase2 just 4 of them are used: PixelBarrel, PixelEndcap, TOB, TID.
@lowette Regarding specifically this part of the code, I would suggest to use just the function layer() which should return the correct number of the layer/disk. You can check it out here. Actually I can also see that there are two functions that do exactly what you ask here, but I would strongly suggest to not use them. @boudoul do you know why they where added? @venturia, do you agree that the TrackerTopology is not the class that they should be or it was maybe your suggestion? I might miss something..

I agree that those methods in TrackerTopology can be source of confusion and pain in future. Whatever depends on the geometry has to be in TrackerGeometry and not in TrackerTopology.
I also agree that TrackerTopology::layer() has to be used in this case.
I also suggest to use the enumerators like StripSubdetector::TIB as less as possible. They do not contain anymore any information about the actual geometry of the subdetector: they are just integers from 1 to 6. They have to be used only to decide which TrackerTopology method has to be called (as it is correctly done in this case but it is not needed because of the generic layer() method). If you have to know what is the subdetector you should use the methods in TrackerGeometry here: https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/Geometry/TrackerGeometryBuilder/interface/TrackerGeometry.h#L81

I switch to use side() and layer() now.

boudoul · 2016-11-01T11:14:26Z

Can you make a more sensitive title, something like introduction of Lorentz Angle correction in Phase2 CPE

cvuosalo · 2016-11-01T17:36:30Z

@ebrondol: Could you please provide some tracking validation results for this PR?

ebrondol · 2016-11-02T17:03:21Z

Dear @cvuosalo , I put the different comparison between with the new CPE and old CPE here for ttbar and 4muons. I do not see any difference in the eff/fakerates plots as well as other distribution that I have checked. The only distribution that significantly changes is the chi2 distribution, where the behavior as a function of eta seems to be better as expected, while the new shape of the overall chi2 distribution is not completely clear to me. It seems now that the uncertainty are now overestimated. @delaere @lowette

cmsbuild · 2016-11-02T23:21:07Z

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

…istinguish between pixel and strip sensors.

cmsbuild · 2016-11-16T13:46:55Z

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

lowette · 2016-11-16T13:48:46Z

Ok, I managed to find back my code changes and inserted them again. From my side I'm done. I hope things are fine again now.

mmusich · 2016-11-16T13:51:10Z

@cmsbuild please test
they do, with the huge advantage of not having 100 merges in your commit history...

cmsbuild · 2016-11-16T13:51:24Z

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

cmsbuild · 2016-11-16T17:47:32Z

+1
Tested at: c8ea040
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16381/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
b35cfcf
83e409f
58f4ff3
57a74ee
d2f7bcb
e796093
0ceca72
902b422
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16381/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16381/git-merge-result

cmsbuild · 2016-11-16T17:47:35Z

Comparison job queued.

cmsbuild · 2016-11-16T18:57:27Z

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

Alternative comparison was/were failed for workflow(s):
4.22

cmsbuild · 2016-11-16T18:58:11Z

-1

Tested at: 01d1082

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
b35cfcf
83e409f
58f4ff3
57a74ee
d2f7bcb
e796093
0ceca72
902b422
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16380/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16380/git-merge-result

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

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
b35cfcf
83e409f
58f4ff3
57a74ee
d2f7bcb
e796093
0ceca72
902b422
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16380/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16392/16380/git-merge-result

cmsbuild · 2016-11-16T18:58:13Z

Comparison job queued.

cmsbuild · 2016-11-16T20:00:40Z

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

Alternative comparison was/were failed for workflow(s):
135.4

cvuosalo · 2016-11-16T21:15:10Z

+1

For #16392 c8ea040

Second approval, after minor code revision, which is satisfactory. New Jenkins tests are OK and show the same tiny differences caused by the tracking changes as observed in previous tests.

cmsbuild · 2016-11-16T21:21:09Z

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

davidlange6 · 2016-11-17T07:05:56Z

+1

cmsbuild added this to the Next CMSSW_8_1_X milestone Oct 30, 2016

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 30, 2016

slava77 reviewed Oct 30, 2016

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Oct 30, 2016

cmsbuild added tests-approved and removed tests-started labels Oct 31, 2016

cmsbuild added comparison-available and removed comparison-pending labels Oct 31, 2016

boudoul reviewed Nov 1, 2016

View reviewed changes

lowette changed the title ~~CPE with LA correction on top of Erica's rebase from ebrondol:phase2_movingToCPEwithLA~~ Introduction of Lorentz Angle correction in Phase 2 CPE Nov 2, 2016

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Nov 2, 2016

Get rid of ncolumns in this testing code, using geometry instead to d…

c8ea040

…istinguish between pixel and strip sensors.

cmsbuild added tests-pending and removed tests-started labels Nov 16, 2016

cmsbuild added tests-started and removed tests-pending labels Nov 16, 2016

cmsbuild added tests-approved and removed tests-started labels Nov 16, 2016

cmsbuild added comparison-available and removed comparison-pending labels Nov 16, 2016

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Nov 16, 2016

cmsbuild added orp-approved and removed orp-pending labels Nov 17, 2016

cmsbuild merged commit 8a25e46 into cms-sw:CMSSW_8_1_X Nov 17, 2016

boudoul mentioned this pull request Nov 22, 2016

Introduction of Lorentz Angle correction in Phase 2 CPE [90X] #16724

Merged

Introduction of Lorentz Angle correction in Phase 2 CPE #16392

Introduction of Lorentz Angle correction in Phase 2 CPE #16392

Conversation

lowette commented Oct 30, 2016

cmsbuild commented Oct 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Oct 30, 2016

cmsbuild commented Oct 30, 2016 • edited

cmsbuild commented Oct 31, 2016

cmsbuild commented Oct 31, 2016

cmsbuild commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boudoul Nov 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebrondol Nov 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boudoul commented Nov 1, 2016

cvuosalo commented Nov 1, 2016

ebrondol commented Nov 2, 2016

cmsbuild commented Nov 2, 2016

cmsbuild commented Nov 16, 2016

lowette commented Nov 16, 2016

mmusich commented Nov 16, 2016

cmsbuild commented Nov 16, 2016 • edited

cmsbuild commented Nov 16, 2016

cmsbuild commented Nov 16, 2016

cmsbuild commented Nov 16, 2016

cmsbuild commented Nov 16, 2016

cmsbuild commented Nov 16, 2016

cmsbuild commented Nov 16, 2016

cvuosalo commented Nov 16, 2016

cmsbuild commented Nov 16, 2016

davidlange6 commented Nov 17, 2016

cmsbuild commented Oct 30, 2016 •

edited

boudoul Nov 2, 2016 •

edited

ebrondol Nov 2, 2016 •

edited

cmsbuild commented Nov 16, 2016 •

edited