Skip to content
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

Disable SETMuon and caloMuon from the standard reconstruction #14747

Merged
merged 8 commits into from Jul 5, 2016

Conversation

jhgoh
Copy link
Contributor

@jhgoh jhgoh commented Jun 2, 2016

SETMuons and CaloMuons have been decided in 2015 [1] to be removed from the standard reconstruction, based on the survey made in the end of 2014 [2,3].

These are configuration file level change to remove SETMuons from the standard reconstruction sequence and also remove them in the Validation.
CaloMuons are kept in the reconstruction sequence, but it is de-activated by increasing the minimum pT threshold to very large value (1e9) as was done in #9559 to speed up HI reconstruction.

[1] https://indico.cern.ch/event/453360/contributions/1963025/attachments/1168716/1688380/Muon_POG_News_12102015.pdf
[2] https://hypernews.cern.ch/HyperNews/CMS/get/muon/1140.html
[3] https://hypernews.cern.ch/HyperNews/CMS/get/muon/1069.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2016

A new Pull Request was created by @jhgoh (Junghwan John Goh) for CMSSW_8_1_X.

It involves the following packages:

RecoMuon/Configuration
RecoMuon/MuonIdentification
Validation/RecoMuon

@cvuosalo, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@battibass, @abbiendi, @bellan, @HuguesBrun, @trocino, @bachtis, @rociovilar this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@slava77
Copy link
Contributor

slava77 commented Jun 2, 2016

@jhgoh
I'm missing where the SET is removed from the sequences.
Probably the unscheduled cleanup will remove it, but why not remove in full.
Also, what is the reason the dummified (pt cut very large) calomuons are still needed?

@slava77
Copy link
Contributor

slava77 commented Jun 6, 2016

@jhgoh
please respond to earlier comments here
#14747 (comment)
I didn't start the tests because of the unclear items.

@jhgoh
Copy link
Contributor Author

jhgoh commented Jun 7, 2016

@slava77 Sorry for late reply.
I grep'ed SETMuon string and tried to remove them.
Maybe I'm missed something so let me check once again.

For the calomuon pt cut, it is to reduce CPU usage of the MuonIdProducer. If CPU time from that module doesn't matter anymore, maybe it is OK to restore to the original value.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2016

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

@jhgoh
Copy link
Contributor Author

jhgoh commented Jun 7, 2016

There was missing commit while I was merging with local head.

@@ -12,7 +12,7 @@
inputMuons = cms.InputTag("muons1stStep"),
inputTracks = cms.InputTag("generalTracks"),
minCaloCompatibility = cms.double(0.6),
minPt = cms.double(1.0)
minPt = cms.double(1E9)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you check if this does anything?
Looking at implementation of CaloMuonProducer (since 2009), most of the configuration parameters are actually bogus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cut was applied on wrong place :-( that had to be applied on MuonIdProducer.

@slava77
Copy link
Contributor

slava77 commented Jun 7, 2016

SET setup makes more sense now with the last commit.

About calomuons, it looks like the cut should be placed in a different place (in MuonIdProducer configuration).
Calomuons module takes zero time, since it just copies and renames muons1stStep
I still want to understand better why calomuons are still needed in the event content and in the standard sequences (RecoMuon/Configuration/python/RecoMuon_EventContent_cff.py and RecoMuon/MuonIdentification/python/muonIdProducerSequence_cff.py).

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2016

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

@cvuosalo
Copy link
Contributor

cvuosalo commented Jun 8, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2016

-1

Tested at: b7a3a7f

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

I found follow errors while testing this PR

Failed tests: RelVals AddOn

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
5.1 step1

runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log
135.4 step1
runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log
  • AddOn:

I found errors in the following addon tests:

cmsDriver.py TTbar_8TeV_TuneCUETP8M1_cfi --conditions auto:run1_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,HLT:@Fake,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot Realistic8TeVCollision : FAILED - time: date Thu Jun 9 00:19:03 2016-date Thu Jun 9 00:18:52 2016 s - exit: 256
cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,HLT:@relval25ns,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_25ns --magField 38T_PostLS1 : FAILED - time: date Thu Jun 9 00:19:15 2016-date Thu Jun 9 00:19:06 2016 s - exit: 256
cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,HLT:@relval2016,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_2016 --magField 38T_PostLS1 : FAILED - time: date Thu Jun 9 00:19:20 2016-date Thu Jun 9 00:19:11 2016 s - exit: 256

@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

cvuosalo commented Jul 5, 2016

+1

For #14747 cc8e8e3

Elimination of obsolete Calo and SET muons.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-06-29-2300 show no significant differences except for the desired disappearance of Calo and SET muons. DT DQM plots using SET muons are now empty, but #15015 will switch them to use stand-alone muons. An extended test of workflow 136.731_RunSinglePh2016B against baseline CMSSW_8_1_0_pre7 also shows the expected disappearance of Calo and SET muons from plots and from RECO event content:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
    160.7 ->         0.0       -161     OLDO  -0.01     recoTracks_standAloneSETMuons__reRECO.
    149.7 ->         0.0       -150     OLDO  -0.01     recoTrackExtras_globalSETMuons__reRECO.
   4133.5 ->         0.0      -4133     OLDO  -0.30     recoCaloMuons_calomuons__reRECO.
    208.1 ->         0.0       -208     OLDO  -0.02     recoTrackExtras_standAloneSETMuons__reRECO.
    126.8 ->         0.0       -127     OLDO  -0.01     recoTracks_globalSETMuons__reRECO.
    953.6 ->         0.0       -954     OLDO  -0.07     TrajectorySeeds_SETMuonSeed__reRECO.
    445.6 ->         0.0       -446     OLDO  -0.03     TrackingRecHitsOwned_globalSETMuons__reRECO.
    166.1 ->         0.0       -166     OLDO  -0.01     recoTracks_standAloneSETMuons_UpdatedAtVtx_reRECO.
    836.8 ->         0.0       -837     OLDO  -0.06     TrackingRecHitsOwned_standAloneSETMuons__reRECO.
-------------------------------------------------------------
  1367891 ->     1360676      -7215            -0.5     ALL BRANCHES

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2016

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, @Degano, @smuzaffar

@slava77
Copy link
Contributor

slava77 commented Jul 5, 2016

On 7/5/16 12:30 PM, Carl Vuosalo wrote:

DT DQM plots using SET muons are now empty, but #15015
#15015 will switch them to use
stand-alone muons.

8c3b928
from this PR
was supposed to address this already
(it changes DQM/DTMonitorModule/python/dtChamberEfficiency_cfi.py in
the same way as #15015)
What's missing?

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b785e61 into cms-sw:CMSSW_8_1_X Jul 5, 2016
@cvuosalo
Copy link
Contributor

cvuosalo commented Jul 5, 2016

hold

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2016

Pull request has been put on hold by @cvuosalo
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cvuosalo
Copy link
Contributor

cvuosalo commented Jul 5, 2016

@jhgoh: I see now you included #15015 in this PR, but the DT DQM plots are still empty. What is going wrong?

This PR is already merged, so we'll need a new one for the fix.

@fcavallo
Copy link
Contributor

fcavallo commented Jul 6, 2016

yes sorry I'll do it today! I just realized yesterday that there is another parameter to be changed in order to get with standAloneMuons the same results as before with SETMuons. I was about to make another PR... do you have a different advice?

@jhgoh
Copy link
Contributor Author

jhgoh commented Jul 6, 2016

@fcavallo thank you for the information. I was little but puzzled on this issue but anyway it's OK if you already have solution.

@slava77
Copy link
Contributor

slava77 commented Jul 6, 2016

@cvuosalo
please post CPU changes from this PR.

@cvuosalo
Copy link
Contributor

cvuosalo commented Jul 6, 2016

@fcavallo: I think we need a new PR for 81X, but you can also put the fix into #15015 that is going into 80X.

@jhgoh jhgoh deleted the RemoveSETMuonCaloMuon branch August 4, 2016 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants