New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Time for layerClusters in HGCAL #24838

Merged

cmsbuild merged 4 commits into cms-sw:master from amartelli:time2DClHGCAL

Oct 16, 2018

Contributor

amartelli commented Oct 9, 2018

produce a ValueMap with the computed time for 2d clusters, so the BasicCluster dataformat is not modified

arabella added 2 commits

October 9, 2018 17:20


          valueMap with time for layerclusters

2a0ccaa


          fix comments

5f2d116

cmsbuild added this to the CMSSW_10_3_X milestone

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels

Contributor

cmsbuild commented Oct 9, 2018

The code-checks are being triggered in jenkins.

Contributor

cmsbuild commented Oct 9, 2018

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24838/6789

cmsbuild added code-checks-approved and removed code-checks-pending labels

Contributor

cmsbuild commented Oct 9, 2018

A new Pull Request was created by @amartelli for master.

It involves the following packages:

RecoLocalCalo/HGCalRecProducers

@perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@edjtscott, @vandreev11, @sethzenz, @felicepantaleo, @rovere, @lgray, @cseez, @pfs, @kpedro88 this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

Contributor

slava77 commented Oct 9, 2018

@cmsbuild please test

Contributor

cmsbuild commented Oct 9, 2018 •

edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30945/console Started: 2018/10/09 17:38

cmsbuild added tests-started and removed tests-pending labels

slava77 reviewed

View reviewed changes

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

                 switch(algoId){
                 case reco::CaloCluster::hgcal_em:
                   evt.getByToken(hits_ee_token,ee_hits);
                   algo->populate(*ee_hits);
+                  for(auto it: *ee_hits) hitmap[it.detid()] = it.time();

Contributor

slava77 Oct 9, 2018

auto const&

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

                   break;
                 case  reco::CaloCluster::hgcal_had:
                   evt.getByToken(hits_fh_token,fh_hits);
                   evt.getByToken(hits_bh_token,bh_hits);
                   if( fh_hits.isValid() ) {
                     algo->populate(*fh_hits);
+                    for(auto it: *fh_hits) hitmap[it.detid()] = it.time();

Contributor

slava77 Oct 9, 2018

auto const&

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

                   } else if ( bh_hits.isValid() ) {
                     algo->populate(*bh_hits);
                   }
                   break;
                 case reco::CaloCluster::hgcal_mixed:
                   evt.getByToken(hits_ee_token,ee_hits);
                   algo->populate(*ee_hits);
+                  for(auto it: *ee_hits){

Contributor

slava77 Oct 9, 2018

auto const&

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

                   evt.getByToken(hits_fh_token,fh_hits);
                   algo->populate(*fh_hits);
+                  for(auto it: *fh_hits){

Contributor

slava77 Oct 9, 2018

auto const&

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

+                  if(sCl.size() >= 3){
+                    std::vector<float> timeClhits;
+                    for(auto hit : sCl.hitsAndFractions()){

Contributor

slava77 Oct 9, 2018

auto const&

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

+                    std::vector<float> timeClhits;
+                    for(auto hit : sCl.hitsAndFractions()){
+                      std::map<const HGCalDetId, float>::iterator finder = hitmap.find(hit.first);

Contributor

slava77 Oct 9, 2018

const_iterator or auto

kpedro88 reviewed

View reviewed changes

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

+                //make a map detid-rechit
+                // NB for the moment just host EE and FH hits
+                // timing in digi for BH not implemented for now
+                std::map<const HGCalDetId, float> hitmap;

Contributor

kpedro88 Oct 9, 2018

probably worthwhile to use an unordered_map here

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClusterProducer.cc Outdated

		}

		std::unique_ptr<edm::ValueMap<float>> timeCl(new edm::ValueMap<float>());

Contributor

kpedro88 Oct 9, 2018

auto timeCl = std::make_unique<edm::ValueMap<float>>();

Contributor

cmsbuild commented Oct 9, 2018

+1
Tested at: 5f2d116
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24838/30945/summary.html

Contributor

cmsbuild commented Oct 9, 2018

Comparison job queued.

cmsbuild added tests-approved and removed tests-started labels

Contributor

cmsbuild commented Oct 9, 2018

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3162979
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3162781
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 134 log files, 14 edm output root files, 32 DQM output files

cmsbuild added comparison-available and removed comparison-pending labels

smuzaffar modified the milestones: CMSSW_10_3_X, CMSSW_10_4_X

Contributor

kpedro88 commented Oct 10, 2018

please test

Contributor

cmsbuild commented Oct 10, 2018

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24838/6797

cmsbuild added code-checks-approved and removed code-checks-pending labels

Contributor

cmsbuild commented Oct 10, 2018 •

edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30963/console Started: 2018/10/10 16:07

Contributor

cmsbuild commented Oct 10, 2018

Pull request #24838 was updated. @perrotta, @cmsbuild, @kpedro88, @slava77 can you please check and sign again.

cmsbuild added tests-started and removed tests-pending labels

Contributor

cmsbuild commented Oct 10, 2018

+1
Tested at: 477690f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24838/30963/summary.html

Contributor

cmsbuild commented Oct 10, 2018

Comparison job queued.

cmsbuild added tests-approved and removed tests-started labels

Contributor

cmsbuild commented Oct 10, 2018

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3162979
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3162780
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 134 log files, 14 edm output root files, 32 DQM output files

cmsbuild added comparison-available and removed comparison-pending labels

Contributor

kpedro88 commented Oct 11, 2018

@amartelli I looked at one of the output ROOT files from the matrix tests. The ValueMap is in the event content as expected. 95% of entries have the -99 default value, while the remaining 5% have values mostly between -1 and 1 with a small tail beyond 1. Is this the expected behavior?

Contributor

slava77 commented Oct 11, 2018

+1

for #24838 477690f

code changes are in line with the PR description
jenkins tests pass and comparisons with the baseline (of previously known quantities) show no differences
based on a local copy of the jenkins outputs shows that the new valueMap hgcalLayerClusters_timeLayerCluster shows up in outputs of the 2023 workflows and the values are filled with non-default value for a fraction of the clusters (the code fills non-default values only for 3+ measurements in a cluster; most data in the jenkins tests is noise and should have small clusters)
e.g. wf 20034 D17

or with the default suppressed

cmsbuild added reconstruction-approved and removed reconstruction-pending labels

Contributor

kpedro88 commented Oct 15, 2018

+upgrade
@amartelli please confirm the output that @slava77 and I have observed is correct

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels

Contributor

cmsbuild commented Oct 15, 2018

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

Contributor

fabiocos commented Oct 16, 2018

+1

cmsbuild added orp-approved and removed orp-pending labels

cmsbuild merged commit 49628e2 into cms-sw:master

Contributor Author

amartelli commented Oct 16, 2018

@kpedro88 yes what is observed is correct. The non default values are indeed filled for 3+ measurements with time in a cluster. sorry for the delay and thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment