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

Adding digi-based shower tagging to reco::Muon #26369

Merged
merged 9 commits into from Apr 18, 2019

Conversation

battibass
Copy link

PR description:

This PR adds to the muon object a variable that counts the number of digis per station that are collected in the vicinity of an extrapolated track. Based on this variable, member functions are added to reco::Muon to tag showers based on digi information similarly to what is done , for example, in this study.

Besides the addition of one more variable to MuonChamberMatch (which propagates up to miniAOD), no change to reco::Muon objects expected.

PR validation:

The consistency of the reco::Muon::nDigisInStation method with the digi counting made a posteriori in ntuples (used for studies like the ones linked above) has been tested on 1K events of a flat pT muon gun and is available here, in such sample. No discrepancy between the two sources of information was found.

Event dumps were used to test that reco::Muon::hasShowerInStation and reco::Muon::numberOfShowers behave as expected.

The increase in event size, for slimmedMuons in miniAOD and for the workflow 136.891, was checked to be less than 1%.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26369/9108

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

A new Pull Request was created by @battibass (Carlo Battilana) for master.

It involves the following packages:

DataFormats/MuonReco
RecoMuon/MuonIdentification

@cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks.
@bellan, @folguera, @abbiendi, @rovere, @jhgoh, @echapon, @calderona, @HuguesBrun, @drkovalskyi, @trocino, @amagitte, @bachtis, @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

@perrotta
Copy link
Contributor

perrotta commented Apr 5, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33998/console Started: 2019/04/05 14:57

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

-1

Tested at: 4ad4961

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

I found follow errors while testing this PR

Failed tests: UnitTests RelVals

  • Unit Tests:

I found errors in the following unit tests:

---> test testTauEmbeddingProducers had ERRORS

  • RelVals:

When I ran the RelVals I found an error in the following workflows:
101.0 step1

runTheMatrix-results/101.0_SingleElectronE120EHCAL+SingleElectronE120EHCAL/step1_SingleElectronE120EHCAL+SingleElectronE120EHCAL.log

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

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

@fabiocos
Copy link
Contributor

fabiocos commented Apr 5, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34018/console Started: 2019/04/05 22:37

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 18, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34272/console Started: 2019/04/18 13:30

@fabiocos
Copy link
Contributor

@davidlange6 the point is that I do not see any failure in the report, let's try it again...

@mrodozov
Copy link
Contributor

@slava77 now I see your question, only "abort" or "please abort" should do. from time to time we have this github webhook failures and that might be it, but I'll assume the bot got confused from your command

@mrodozov
Copy link
Contributor

@fabiocos I'm searching in the logs and it says

00:11:09 ++ grep -i 'had errors\|recipe for target' /build/cmsbld/jenkins/workspace/ib-any-integration/unitTests.log
00:11:09 ++ sed 's|'\''||g;s|.*recipe for target *||;s|.*unittests_|---> test |;s| failed$| timeout|'
00:11:09 + TEST_ERRORS=
00:11:09 ++ grep -i 'had errors' /build/cmsbld/jenkins/workspace/ib-any-integration/unitTests.log
00:11:09 + TEST_ERRORS=
00:11:09 + true
00:11:09 ++ grep ALL_OK /build/cmsbld/jenkins/workspace/ib-any-integration/unitTests.log
00:11:09 + GENERAL_ERRORS=
00:11:09 + true
00:11:09 Errors in the unit tests
00:11:09 + '[' X '!=' X -o X = X ']'
00:11:09 + echo 'Errors in the unit tests'
00:11:09 + echo 'UNIT_TEST_RESULTS;ERROR'
00:11:09 + ALL_OK=false
00:11:09 + UNIT_TESTS_OK=false

which in fact is not true, there are no test failing.
this part :

TEST_ERRORS=
00:11:09 ++ grep -i 'had errors' /build/cmsbld/jenkins/workspace/ib-any-integration/unitTests.log
00:11:09 + TEST_ERRORS=
00:11:09 + true

that sets the TEST_ERRORS to true doesn't make any sense, greps for any error, there is none, yet still returns true. lets see if it persists.

@fabiocos
Copy link
Contributor

@civanch @santocch the SIM and ANA part of this PR is restricted to the Tau embedding configuration where a new flag introduced is set to False by default...

@cmsbuild
Copy link
Contributor

@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-26369/34272/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-26369/29034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D41+RecoFullGlobal_2023D41+HARVESTFullGlobal_2023D41

Comparison Summary:

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

@fabiocos
Copy link
Contributor

@civanch given the latest changes I will take your previous signature as valid, anyway please check and sign it again for future reference, or comment in case

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 593f863 into cms-sw:master Apr 18, 2019
ShowerDigiFillerParameters = cms.PSet(
digiMaxDistanceX = cms.double(25.0),
dtDigiCollectionLabel = cms.InputTag("muonDTDigis"),
cscDigiCollectionLabel = cms.InputTag("muonCSCDigis","MuonCSCStripDigi")
Copy link
Contributor

Choose a reason for hiding this comment

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

@battibass @slava77 it looks like this line is creating issues in wf 1102.0 step4 RECOFROMRECO:

   [7] Prefetching for module TrackProducer/'mergedDuplicateTracks'
   [8] Prefetching for module DuplicateTrackMerger/'duplicateTrackCandidates'
   [9] Prefetching for module TrackCollectionMerger/'preDuplicateMergingGeneralTracks'
   [10] Prefetching for module TrackProducer/'muonSeededTracksInOut'
   [11] Prefetching for module CkfTrackCandidateMaker/'muonSeededTrackCandidatesInOut'
   [12] Prefetching for module MuonReSeeder/'muonSeededSeedsInOut'
   [13] Calling method for module MuonIdProducer/'earlyMuons'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: MuonDigiCollection<CSCDetId,CSCStripDigi>
Looking for module label: muonCSCDigis
Looking for productInstanceName: MuonCSCStripDigi

   Additional Info:
      [a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

----- End Fatal Exception -------------------------------------------------

This clearly needs to be fixed, my question is: if this is not affecting any other standard workflow as it seems, and it is not preventing the basic validation of pre4, can we consider to postpone its fix after pre4 is built? Or do you see further possible issues?

Switching this module off in the configuration allows also the step4 of 1102 to run without problems.

Copy link
Author

Choose a reason for hiding this comment

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

@fabiocos , @slava77 can surely reply more accurately than I can.
In principle this PR implies that digis are available (hence implies that unpacking was done starting from RAW). For any workflow for which this is the case there should be no problem.
The question is, are there RelVal workflows where this is not the case?
I'm happy to work on a patch asap, but I would need inputs to understand better how I should be testing it, as my check with runTheMatrix.py -l limited -i all clearly failed to spot this issue, as well as the one with TauAnalysis/MCEmbeddingTools/ that was instead spot during integration ...

Copy link
Contributor

Choose a reason for hiding this comment

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

@battibass indeed this is a bit special case, and only wf 1102.0 implementing it shows this issue. I see that @slava77 has already mentioned it in #26500 .

@slava77
Copy link
Contributor

slava77 commented Apr 19, 2019 via email

@battibass
Copy link
Author

@slava77 I understand well that I can run runTheMatrix.py -l 1102.0 my question was more general: is there a wider set of tests that I should perform? If so which one?

In principle, one could use dt1DRecHits, for the DTs. They come with left/right ambiguities (there are actually 2 recHits per digi), but it can be handled. For CSC I think the situation is more complex. From a discussion I had with @ptcox when starting all this, the interpretation of csc2DRecHits is not trivial as both clustering and combination of wire and strip digi information happens at RecHit building, and it is not straightforward to understand what the counting of csc2DRecHits actually means for cases beyond the ones where you have clear muon track segments. The use of CSC strip digis was in fact chosen as it should be a better proxy for "counting" the activity in a chamber in case a shower occur.

@slava77
Copy link
Contributor

slava77 commented Apr 19, 2019

as discussed in the RECO meeting, we can have a quick solution to simply add the CSC and DT digis to the RECO event content.
If there are other reasons to wait with the build of pre4, perhaps this solution can be provided today.

A better choice perhaps is to allow modifications to module PSets of the RECOfromRECO workflows (which is not possible now). See #26500.
We better complete this on the time scale of pre5.

@santocch
Copy link

+1

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