[10.0.x] Miscellaneous updates in Alignment PV Validation #21078

mmusich · 2017-10-30T15:05:25Z

Greetings,
this is the summary of the updates proposed in this PR:

move specialized track selection (most hits/tracks selection cuts are open w.r.t. the rest of validation types) from the PV validation configuration file template to the common track selection and refitting sequence.
use configurable (log)-binning for pT-binned resolution and bias.
update plotting macros to check for consistent pT-binning among input files.
update example ini file using 2016 legacy data and 2017 MC with final pixel geometry.
updated reference file address with consistently updated binning
update unit test to use the common track selection and refitting sequence
few graphical improvements on the output plots.

cmsbuild · 2017-10-30T15:06:07Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-10-30T15:08:52Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21078/1712

cmsbuild · 2017-10-30T15:09:11Z

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

Alignment/CommonAlignment
Alignment/OfflineValidation

@ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @lpernie can you please review it and eventually sign? Thanks.
@ghellwig, @pakhotin, @tocheng, @tlampen, @mschrode, @mmusich this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

arunhep · 2017-10-30T22:15:15Z

please test

cmsbuild · 2017-10-30T22:15:35Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24064/console Started: 2017/10/30 23:16

cmsbuild · 2017-10-31T00:42:14Z

+1
Tested at: e8a5f25
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21078/24064/summary.html

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

cmsbuild · 2017-10-31T00:42:17Z

Comparison job queued.

cmsbuild · 2017-10-31T02:54:20Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2838442
DQMHistoTests: Total failures: 115
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2838156
DQMHistoTests: Total skipped: 171
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 10 edm output root files, 26 DQM output files

ghellwig · 2017-11-08T11:28:48Z

Alignment/CommonAlignment/python/tools/trackselectionRefitting.py

+                "d0Max" :  999999.0,
+                "dzMin" : -999999.0,
+                "dzMax" :  999999.0
+                })


@mmusich did you consider the altenative approach of using the common sequence first and overriding in your config as a second step similar to this:

process.refittingSequence = getSequence(...) process.HighPurityTrackSelector.pMin = 0.0 ...

This would avoid further cluttering of the common method, but I'm also fine with your approach.

@ghellwig, that's exactly how it was before (https://github.com/cms-sw/cmssw/pull/21078/files/e8a5f259ec7056352d1c24bbb9c0ea5b2dba01f7#diff-72d8a78a6ccf3dc6cc176329075a4d3fL3) and I don't like it since the list of things to be changed is large.

ok, as I said, I am also fine with this way

ghellwig · 2017-11-08T11:30:38Z

Alignment/OfflineValidation/interface/PVValidationHelpers.h

+    }
+    return ret;
+  }
+


shouldn't double be replaced with T here?

Hi @ghellwig, fixed in df904d2

Hi @mmusich, this actually applies to all occurences of double.

or do you want to ensure that double is used internally?

not really, everything is template type now.

ok, thanks
not sure, if it should be const T& in the function argument, but for the types it is intended for this should not really matter

ghellwig · 2017-11-08T11:35:04Z

Alignment/OfflineValidation/plugins/PrimaryVertexValidation.h

@@ -167,7 +174,9 @@ class PrimaryVertexValidation : public edm::one::EDAnalyzer<edm::one::SharedReso
  // pT binning as in paragraph 3.2 of CMS-PAS-TRK-10-005 (https://cds.cern.ch/record/1279383/files/TRK-10-005-pas.pdf)

  //                                      0   1   2   3   4   5   6   7   8   9  10  11  12  13  14  15  16  17  18  19  20  21  22  23  24  25  26  27  28  29  30  31  32  33  34  35  36   37  38   39  40  41  42  43  44  45   46  47  48
-  const float mypT_bins_[nPtBins_+1] = {0.5,0.6,0.7,0.8,0.9,1.0,1.1,1.2,1.3,1.4,1.5,1.6,1.7,1.8,1.9,2.0,2.1,2.2,2.3,2.4,2.5,2.6,2.7,2.8,2.9,3.0,3.1,3.2,3.3,3.4,3.5,3.6,3.7,3.8,3.9,4.0,4.1,4.25,4.5,4.75,5.0,5.5,6.0,7.0,8.0,9.0,11.0,14.0,20.}; 
+  //const float mypT_bins_[nPtBins_+1] = {0.5,0.6,0.7,0.8,0.9,1.0,1.1,1.2,1.3,1.4,1.5,1.6,1.7,1.8,1.9,2.0,2.1,2.2,2.3,2.4,2.5,2.6,2.7,2.8,2.9,3.0,3.1,3.2,3.3,3.4,3.5,3.6,3.7,3.8,3.9,4.0,4.1,4.25,4.5,4.75,5.0,5.5,6.0,7.0,8.0,9.0,11.0,14.0,20.}; 
+


can these comments be removed?

I prefer to keep them, for reference.

ghellwig · 2017-11-08T11:35:21Z

Alignment/OfflineValidation/python/TkAlAllInOneTool/configTemplates.py

@@ -163,6 +163,7 @@
 CommonTrackSelectionRefitting = """
 import Alignment.CommonAlignment.tools.trackselectionRefitting as trackselRefit
 process.seqTrackselRefit = trackselRefit.getSequence(process, '.oO[trackcollection]Oo.',
+                                                     isPVValidation=.oO[ispvvalidation]Oo., 


see #21078 (comment)

ghellwig · 2017-11-08T11:35:46Z

Alignment/OfflineValidation/python/TkAlAllInOneTool/genericValidation.py

@@ -519,6 +519,7 @@ def getRepMap(self, alignment=None):
            "istracksplitting": str(isinstance(self, TrackSplittingValidation)),
            "cosmics0T": str(self.cosmics0T),
            "use_d0cut": str(self.use_d0cut),
+            "ispvvalidation": str(self.isPVValidation) 


see #21078 (comment)

ghellwig · 2017-11-08T11:35:53Z

Alignment/OfflineValidation/python/TkAlAllInOneTool/genericValidation.py

@@ -535,6 +536,9 @@ def getRepMap(self, alignment=None):
    def use_d0cut(self):
        return "Cosmics" not in self.general["trackcollection"]  #use it for collisions only
    @property
+    def isPVValidation(self):


see #21078 (comment)

ghellwig · 2017-11-08T11:36:29Z

Alignment/OfflineValidation/python/TkAlAllInOneTool/primaryVertexValidation.py

@@ -48,6 +49,10 @@ def use_d0cut(self):
        return False

    @property
+    def isPVValidation(self):


see #21078 (comment)

ghellwig · 2017-11-08T11:38:00Z

Alignment/OfflineValidation/test/test_all_cfg.py

+# process.FinalTrackRefitter.src = "generalTracks"
+# process.FinalTrackRefitter.TrajectoryInEvent = True
+# process.FinalTrackRefitter.NavigationSchool = ''
+# process.FinalTrackRefitter.TTRHBuilder = "WithAngleAndTemplate"


please remove the commented code

As I am still not totally convinced that the CTSR sequence is OK to use in all corner use cases I prefer to keep track of the standard refitter configuration (at least in the test cfg: in the template of the all-ini-one it is removed)

cmsbuild · 2017-11-08T14:30:32Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-11-08T14:33:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21078/1871

cmsbuild · 2017-11-08T14:34:13Z

Pull request #21078 was updated. @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @lpernie can you please check and sign again.

mmusich · 2017-11-08T14:36:06Z

@ghellwig if you are fine with the implementation of the review, please restart the tests, otherwise I'll wait for more comments. Thank you!

davidlange6 · 2017-11-08T15:42:52Z

please test

cmsbuild · 2017-11-08T15:43:13Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24276/console Started: 2017/11/08 18:41

ghellwig · 2017-11-08T15:47:33Z

@mmusich I am fine with it now
@davidlange6 thanks for triggering the tests

ghellwig · 2017-11-08T16:13:15Z

+1
provided jenkins agrees

cmsbuild · 2017-11-08T16:13:40Z

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

cmsbuild · 2017-11-08T19:48:25Z

+1
Tested at: 159f211
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21078/24276/summary.html

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

cmsbuild · 2017-11-08T19:48:27Z

Comparison job queued.

cmsbuild · 2017-11-08T23:01:49Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2903670
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2903498
DQMHistoTests: Total skipped: 171
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram bins added: 0.0 ( 22 files compared)
Checked 107 log files, 8 edm output root files, 26 DQM output files

davidlange6 · 2017-11-14T09:27:21Z

+1

cmsbuild added this to the CMSSW_10_0_X milestone Oct 30, 2017

cmsbuild added alca-pending code-checks-pending comparison-pending orp-pending pending-signatures tests-pending labels Oct 30, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 30, 2017

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

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

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

ghellwig reviewed Nov 8, 2017

View reviewed changes

mmusich added 6 commits November 8, 2017 12:50

Change reference file as consequence of cms-sw#19839

2e1a2c1

fix xrdcp copies in pv validation template scripts

43d4755

update ini file for 2017 Data/MC

b696aee

Minor graphical improvements

a7a2113

complete example ini file

09df5af

Add automatic log binning for pT-binning plots

98baee3

cmsbuild added code-checks-pending tests-pending labels Nov 8, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 8, 2017

cmsbuild added tests-started and removed tests-pending labels Nov 8, 2017

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Nov 8, 2017

cmsbuild added tests-approved and removed tests-started labels Nov 8, 2017

cmsbuild added comparison-available and removed comparison-pending labels Nov 8, 2017

cmsbuild added orp-approved and removed orp-pending labels Nov 14, 2017

cmsbuild merged commit 2a6f759 into cms-sw:master Nov 14, 2017

[10.0.x] Miscellaneous updates in Alignment PV Validation #21078

[10.0.x] Miscellaneous updates in Alignment PV Validation #21078

Conversation

mmusich commented Oct 30, 2017

cmsbuild commented Oct 30, 2017

cmsbuild commented Oct 30, 2017

cmsbuild commented Oct 30, 2017

arunhep commented Oct 30, 2017

cmsbuild commented Oct 30, 2017 • edited

cmsbuild commented Oct 31, 2017

cmsbuild commented Oct 31, 2017

cmsbuild commented Oct 31, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Nov 8, 2017

cmsbuild commented Nov 8, 2017

cmsbuild commented Nov 8, 2017

mmusich commented Nov 8, 2017

davidlange6 commented Nov 8, 2017

cmsbuild commented Nov 8, 2017 • edited

ghellwig commented Nov 8, 2017

ghellwig commented Nov 8, 2017

cmsbuild commented Nov 8, 2017

cmsbuild commented Nov 8, 2017

cmsbuild commented Nov 8, 2017

cmsbuild commented Nov 8, 2017

davidlange6 commented Nov 14, 2017

cmsbuild commented Oct 30, 2017 •

edited

cmsbuild commented Nov 8, 2017 •

edited