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

[GEM] Change digitisation by using geant energy deposition of simHit #37285

Merged
merged 3 commits into from May 11, 2022

Conversation

seulgi324
Copy link

PR description:

  • Change the digistation by using geant energy deposition of simHit
  • Some non muon particles were not correctly implemented
  • Allows exotic particles to be digitised
  • Remove the parameters not used
    • muonPdgId, cspeed, elecMomCut1, elecMomCut2, elecEffLowCoeff, elecEffLowParam0, elecEffMidCoeff, elecEffMidParam0, elecEffMidParam1
  • presented at GEM DPG meeting

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37285/28913

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • SimMuon/GEMDigitizer (upgrade, simulation)

@cmsbuild, @AdrianoDee, @srimanob, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@jshlee, @giovanni-mocellin, @jhgoh, @slomeo, @watson-ij this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@civanch
Copy link
Contributor

civanch commented Mar 22, 2022

please test

@seulgi324 seulgi324 changed the title Change the digistation by using geant energy deposition of simHit Change the GEM digistation by using geant energy deposition of simHit Mar 22, 2022
@seulgi324 seulgi324 changed the title Change the GEM digistation by using geant energy deposition of simHit [GEM] Change digistation by using geant energy deposition of simHit Mar 22, 2022
@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-35413d/23268/summary.html
COMMIT: 09e1e32
CMSSW: CMSSW_12_4_X_2022-03-21-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37285/23268/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

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

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-35413d/39434.911_TTbar_14TeV+2026D88_DD4hep+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 949 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3695650
  • DQMHistoTests: Total failures: 58987
  • DQMHistoTests: Total nulls: 59
  • DQMHistoTests: Total successes: 3636582
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Mar 23, 2022

@seulgi324 , it is natural that this MR change results. Can you confirm that modifications are expected?

@jshlee
Copy link
Contributor

jshlee commented Mar 24, 2022

@civanch It looks like a lot more failed than expected.
We expect only GEM digis to slightly increase.

@srimanob
Copy link
Contributor

srimanob commented Mar 24, 2022

@jshlee
Failure in comparison of 39434.911 can be ignored. It is known, but still need sometimes to understand. I think you can focus on changes of, i.e. 39434.0.

@jshlee
Copy link
Contributor

jshlee commented Mar 24, 2022

@srimanob 39434.0 shows exactly the changes that we expect.
We get more GEM digis due to electrons as demonstrated in @seulgi324 talk.
Also, since we remove the random number generator selecting which simhits to make digis, we expect some fluctuations.

@srimanob
Copy link
Contributor

OK, great. I assume the change can be seen also in Run-3 workflow (e.g. 11634), right?

@jshlee
Copy link
Contributor

jshlee commented Mar 24, 2022

Yes, apart from 11634.911

@civanch
Copy link
Contributor

civanch commented Mar 26, 2022

+1

Except 39434.0 other minor modifications are expected

@srimanob
Copy link
Contributor

Hi @civanch
What I don't understand is why it seems to appear in few workflows, instead of overall wfs that have GEM.

@srimanob
Copy link
Contributor

However, we should converge. If experts say it is expected, I can sign after the PR test (just to make a fresh test before merge).

@jshlee
Copy link
Contributor

jshlee commented May 10, 2022

@civanch @srimanob the changes for gem are exactly what we want. However, this change looks like it's changing muons which get propagated to many objects and we should try to understand this.

@civanch
Copy link
Contributor

civanch commented May 10, 2022

Let me comment more: when Geant4 energy deposition is used it includes energy loss fluctuations, which are sampled during tracking. This is done in both cases.

If G4 energy depositions are not used, at DIGI step energy deposition should be sampled with some algorithm, which is different from one at SIM step and some random numbers are used for that. As a result, energy depositions are slightly different. But the effect may be even more if random number sequence at DIGI step may be different. So, it strongly depends on how random number seeds are defined for various DIGI plugins. Because of that, validation with small statistics may be biased. It is a possibility but I am not sure about this.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-35413d/24592/summary.html
COMMIT: e897b9b
CMSSW: CMSSW_12_4_X_2022-05-10-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37285/24592/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 19484 lines to the logs
  • Reco comparison results: 870 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3696633
  • DQMHistoTests: Total failures: 2608
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3694003
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 206 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented May 11, 2022

+1

@perrotta
Copy link
Contributor

Outputs from tests look correct to me, taking also into account their low stat: GEM quantities may change, and in some cases they get propagated to the reconstructed muons, as it should.

On the other hand, if you look at the logs (step3), you can find them overwhelmed by messages like

MSG
%MSG-e GEMDigiSource:  GEMDigiSource:GEMDigiSource 10-May-2022 20:09:40 CEST  Run: 1 Event: 1
WARNING: Cannot find the histogram corresponing to the given key

(see for example https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-35413d/24592/runTheMatrix-results/39434.0_TTbar_14TeV+2026D88+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal/step3_TTbar_14TeV+2026D88+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal.log)

They are in the number of several tens per event, and at the end the RelVal logs are full of them (making them basically useless for any other check one wants to do on these files.

Of course, this is not caused by this PR. But, since there is GEM people around in this thread, before we merge this could you please take them into account and either fix the issue underneath (if relevant) or shut up the warning in the code (if what it is reporting is not important).

By the way, the LogWarning is issued by GEMDQMBase::FindHist, see https://github.com/cms-sw/cmssw/blob/master/DQM/GEM/interface/GEMDQMBase.h#L330

@jshlee
Copy link
Contributor

jshlee commented May 11, 2022

@perrotta thank you for pointing this out. we will add a fix asap.

@perrotta
Copy link
Contributor

@perrotta thank you for pointing this out. we will add a fix asap.

Thank you @jshlee : I opened a github issue to keep track of it, see #37906

@perrotta
Copy link
Contributor

(I wrote "+1" without realizing that this was not signed by @cms-sw/upgrade-l2 yet. I therefore retracted the orp signature by now)

@srimanob
Copy link
Contributor

+Upgrade

As discussed above, the change in digi of GEM introduced in the PR can cause the change in PR test.

@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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 9b05910 into cms-sw:master May 11, 2022
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

6 participants