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

Addition of histograms to the Muon DQM #22456

Merged
merged 5 commits into from Mar 26, 2018
Merged

Conversation

parbol
Copy link
Contributor

@parbol parbol commented Mar 5, 2018

In this pull request I'm adding histograms to the Muon DQM in order to correct/improve three aspects:

  • Try to improve the monitoring of the pixel problem from the muon perspective:
  • Two Monitor Elements per Muon ID have been added in EfficiencyAnalyzer.cc and EfficiencyPlotter.cc showing the efficiency as a function of the phi and eta of the innermost hit of the innertrack of the muon.

  • One Monitor Element per Muon ID (Loose, Medium, Tight) has been added to DiMuonAnalyzer.cc to monitor the ratio between the number of bad hits and good hits in the inner track for events where the two leptons have a mass compatible with the Z mass.

  • Monitor variables used by the Soft Muon MVA ID
  • About 15 new monitor elements have been added to MuonRecoAnalyzer.cc to monitor the variables used by the Soft Muon MVA as requested by the POG.
  • Occupancy plot for monitoring of bad lumis
  • One Occupancy plot per muon ID has been added to MuonRecoAnalyzer.cc to be filled only with events where the DCS flags are not OK.

… variables used by the soft MVA muon and occupancy plots for events with DCS flags set to BAD
@cmsbuild cmsbuild changed the base branch from CMSSW_10_1_X to master March 5, 2018 11:47
@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2018

@parbol, CMSSW_10_1_X branch is closed for direct updates. cms-bot is going to move this PR to master branch.
In future, please use cmssw master branch to submit your changes.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2018

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

It involves the following packages:

DQMOffline/Muon

@vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please review it and eventually sign? Thanks.
@barvic, @bellan, @abbiendi, @jhgoh, @calderona, @HuguesBrun, @drkovalskyi, @ptcox, @battibass, @trocino, @folguera, @rociovilar 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

@jfernan2
Copy link
Contributor

jfernan2 commented Mar 5, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26517/console Started: 2018/03/05 17:19

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2018

-1

Tested at: c48201b

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

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
140.53 step2

runTheMatrix-results/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log

150.0 step3
runTheMatrix-results/150.0_HydjetQ_B12_5020GeV_2018+HydjetQ_B12_5020GeV_2018+DIGIHI2018+RECOHI2018+HARVESTHI2018/step3_HydjetQ_B12_5020GeV_2018+HydjetQ_B12_5020GeV_2018+DIGIHI2018+RECOHI2018+HARVESTHI2018.log

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2018

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2477528
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2477351
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1599.39 KiB( 23 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@jfernan2
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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)

@fabiocos
Copy link
Contributor

+1

adding ~ 78 kB of histograms, not negligible but still manageable IMO

diff on dqmMemoryStats.py output for wf 10824.0

< 2564.04 KiB Muons_miniAOD/MuonRecoAnalyzer
< 2561.48 KiB Muons/MuonRecoAnalyzer

2603.06 KiB Muons_miniAOD/MuonRecoAnalyzer
2600.19 KiB Muons/MuonRecoAnalyzer

@cmsbuild cmsbuild merged commit af82bc6 into cms-sw:master Mar 26, 2018
@@ -264,6 +270,9 @@ void EfficiencyAnalyzer::analyze(const edm::Event & iEvent,const edm::EventSetup
h_allProbes_pt->Fill(muon2->pt());
h_allProbes_eta->Fill(muon2->eta());
h_allProbes_phi->Fill(muon2->phi());
h_allProbes_inner_pt->Fill(muon2->innerTrack()->innerMomentum().Rho());
h_allProbes_inner_eta->Fill(muon2->innerTrack()->innerPosition().Eta());
h_allProbes_inner_phi->Fill(muon2->innerTrack()->innerPosition().Phi());
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes have broken DQM for miniAOD jobs running separately from AOD inputs.
E.g. in workflow 136.7611 running on 1000 events

   [0] Processing  Event run: 277069 lumi: 81 event: 35893405 stream: 0
   [1] Calling method for module EfficiencyAnalyzer/'LooseMuonEfficiencyAnalyzer_miniAOD'
Exception Message:
RefCore: A request to resolve a reference to a product of type 'std::vector<reco::TrackExtra>' with ProductID '2:3025'
can not be satisfied because the product cannot be found.
Probably the branch containing the product is not stored in the input file.

@fabozzi
did we notice the problems already in relvals? Or are we not running these regularly?
jenkins tests will likely miss this problem because it takes 2 muons somewhat consistent with Z->mumu to make it to this problematic part of the code

Copy link
Contributor

Choose a reason for hiding this comment

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

@slava77
there workflows are only tested in IB to maintain the sequence, we do not produce them in the relval set for regular validation campaigns

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. So, there are no relvals which run only miniAOD with DQM included?
How are you planning to test the reminiAOD of 2016?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply. I've been absent the whole weekend. So, if I understand correctly, these jobs are failing when asking for the innerTrack to the Muons in miniAOD jobs. I guess this is because the innerTrack is not there anymore and the reference it's not valid. I have added a "IsminiAOD" boolean in such a way that this operation is not done when we are running a miniAOD workflow. However I am unsure how to test this new code is working. Is it enough to run it over a miniAOD file? Any suggestion? Thanks a lot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we store the inner track inside pat::Muon? Just use pat::Muon::track() call instead. It will check if the track is embedded and if not, it will call innerTrack() method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we store the inner track inside pat::Muon? Just use pat::Muon::track() call instead. It will check if the track is embedded and if not, it will call innerTrack() method.

Only the track is saved, not its TrackExtra.
The TrackExtra is not available in AOD

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added a "IsminiAOD" boolean in such a way that this operation is not done when we are running a miniAOD workflow.

good

However I am unsure how to test this new code is working. Is it enough to run it over a miniAOD file? Any suggestion?

In case 2017 AOD is at hand, I suggest to start from the matrix workflow 1325.51 and substitute the ttbar inputs with a Z->mumu AOD file.
The cmsDriver command with 1K events in the test is
cmsDriver.py step2 --conditions auto:phase1_2017_realistic -s PAT,VALIDATION:@miniAODValidation,DQM:@miniAODDQM --process PAT --era Run2_2017,run2_miniAOD_94XFall17 --eventcontent MINIAODSIM,DQM --runUnscheduled --scenario pp --datatier MINIAODSIM,DQMIO --mc -n 1000 --filein filelist:step1_dasquery.log --fileout file:step2.root

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why DQM would care about parameters at the inner point vs at point of closest approach, which is stored in Track. Also, it gets eta and phi of the inner point. Is it really what is needed? @parbol can you please clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an attempt to have in the DQM variables that might be sensitive to the pixels problem. Probably the same thing can be seen using the point of closest approach (what do you think Dima?), since everything we want to see is some degradation. I'm fine with both solutions: the IsminiAOD switch (already implemented), or changing to the point of closest approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's to understand how deeply the track is going, don't you want then rho of the inner point, instead of momentum? I would expect that direction is good enough from POCA. Only location of the first hit can be interesting. Guys, what do you think?

@slava77
Copy link
Contributor

slava77 commented Apr 13, 2018

@fabiocos
very soon we need to validate the re-miniAOD workflows in the context of the 2016 reminiAOD.
Changes from this PR are blocking validation of these workflows.
Somewhat expedient resolution would be nice (revert is easy but perhaps not so nice for the proponents of the changes).

@cmsbuild cmsbuild modified the milestones: CMSSW_10_1_X, CMSSW_10_2_X Apr 13, 2018
@fabiocos
Copy link
Contributor

@slava77 let me follow-up in a separate issue

if (&(*beamSpot)==NULL) {
refPoint = beamSpot->position();
} else {
cout << "ERROR: No beam sport found!" << endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is appearing e.g. in 20034.0 step3 logs (phase2 workflow). cout should not be used.

Also, I'm not sure I understand the logic that you try to assign the refPoint only if the beamSpot address is null...

@parbol
Copy link
Contributor Author

parbol commented Apr 19, 2018

Hello,

I have made a new branch correcting the issues before:

https://github.com/parbol/cmssw/tree/DQMMuonDCSFlag_v2

In particular it corrects three things:

  • The innerTrack is tested to be "isnonNull()" before being used, otherwise the code is not executed.

  • The logic of the beamspot spotted before is now corrected to uses beamSpot().isValid()

  • One remaining cout has been replaced by a edm::Log

Please, could you let me know to which CMSSW should I send the PR against? CMSSW_10_1_X, 2_X?

Thanks

@kpedro88
Copy link
Contributor

Please make the PR to master branch, which currently corresponds to 10_2_X. You will need to rebase your branch (which is currently based on 10_1_X). It would also be nice to fix the typo "sport" -> "spot" in the printout. I leave it up to the DQM reviewers to decide if it should be backported.

@parbol
Copy link
Contributor Author

parbol commented Apr 19, 2018

Ok, it's done in:

#23015

Thanks

@smuzaffar smuzaffar modified the milestones: CMSSW_10_2_X, CMSSW_10_1_X Nov 26, 2018
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