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

Change of SiStrip cross-talk parameters: improvement of the simulated hits #23621

Merged
merged 4 commits into from Jun 21, 2018

Conversation

mjansova
Copy link
Contributor

This PR follows a request of the strip local reco conveners (@mmusich, @echabert).

It aims to be integrated in 10_2_0_pre6.

This PR contains mainly changes in the configuration files SimGeneral/MixingModule/python/SiStripSimParameters_cfi.py and SimTracker/SiStripDigitizer/python/SiStripDigiSimLink_cfi.py. It will affect the simulation of SiStrip tracker hits leading to a better description of their properties.

The cross-talk parameters (called here "CouplingConstants") describe the charge sharing between neighbor channels in the SiStrip tracker. Those parameters are used by the file SimTracker/SiStripDigitizer/plugins/SiTrivialInduceChargeOnStrips.cc .

Thus only the shape of the simulated and then reconstructed clusters will be affected: change of the seed charge, change of the cluster width. The total cluster charge will almost be unchanged as the parameters only change the charge sharing but not the normalization.

The previous parameters were determined at run I but with ageing, these parameters had to be re-evaluated.

A measurement have been done (VR run with cosmics data) and the results are propagated into the config files.

Data/MC validation have been made to ensure that the new parameters improve the agreement of the cluster seed charge and the cluster width.

The validation plots can be found here: https://indico.cern.ch/event/688867/contributions/3040427/attachments/1670393/2679458/cross_talk2018_validation.pdf

On top of the configuration file, a modification of SimTracker/SiStripDigitizer/plugins/SiTrivialInduceChargeOnStrips.cc has been done to allow the activation of the new parameters through flags.

They are split into two categories: barrel modules (activated via CouplingConstantsRunIIDecB ) and disk/wheels (via CouplingConstantsRunIIDecW ). If the two booleans are set to False, the default simulation will be executed.

By default the flags are turned to False, thus the simulation will not be modified.

We plan to request special reval with the flag turned to True so that we can validate the changes and turn it to True in the next pre-realease.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

SimGeneral/MixingModule
SimTracker/SiStripDigitizer

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@echabert, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @prolay, @ebrondol, @mmusich, @threus, @dgulhan 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

@fabiocos
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 19, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28757/console Started: 2018/06/19 15:01

@@ -50,6 +55,27 @@
CouplingConstantPeakW5 = cms.vdouble(0.968, 0.016),
CouplingConstantPeakW6 = cms.vdouble(0.972, 0.014),
CouplingConstantPeakW7 = cms.vdouble(0.964, 0.018),

Copy link
Contributor

Choose a reason for hiding this comment

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

this block looks a clone of the addition above, would not be better to have it in one place only and include it in both places? Duplications are in general more problematic to maintain and error prone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, there are duplications between the two configuration files, but it was there already before my commit. Then the block you are asking about is for Peak mode, which I did not touch at all and I do not see any duplications there. Could you please clarify your question?

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #23621 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

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

@fabiocos
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 20, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28789/console Started: 2018/06/20 18:28

@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-23621/28789/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10289 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2902133
  • DQMHistoTests: Total failures: 18972
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2882971
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -1.217 KiB( 30 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): -0.190 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 250202.181 ): -0.410 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 25202.0 ): -0.617 KiB SiStrip/MechanicalView
  • Checked 128 log files, 14 edm output root files, 31 DQM output files

@fabiocos
Copy link
Contributor

@mjansova @boudoul are the effects the expected ones? I browsed a few histograms only, and found them in agreement with the previous ones, but of course I just randomly picked a few

@perrotta @slava77 FYI as this could impact the output of data/MC agreement

@echabert
Copy link
Contributor

echabert commented Jun 21, 2018 via email

@civanch
Copy link
Contributor

civanch commented Jun 21, 2018

I agree that it is not good having list of run depending parameters duplicated in different python configuration. If not in this PR but this should be indeed improved.

@civanch
Copy link
Contributor

civanch commented Jun 21, 2018

+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

@echabert thanks for the detailed discussion, I understand that this PR is doing what desired, and that this can be tested more extensively in CMSSW_10_2_0_pre6

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

5 participants