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

Pixel Digitizer: Cluster charge re-weighting for 10_2_X #22911

Merged

Conversation

schuetzepaul
Copy link
Contributor

This replaces #22470 .

Pixel Digitizer for simulated data:

The possibility to apply a re-weighting to the charge was added for a proper modelling of radiation damage in the silicon sensors. The re-weighting is based on the 2D cluster templates for the Phase 1 Pixel Detector introduced in #22458 , which is now merged. In addition to changes in the digitizer part, a bug was fixed in the template definition.

The newly added code is by default turned off, a flag in the configuration is needed to run it. Therefore the changes in the digitizer are fully backwards compatible.
We still open a PR for the upcoming release in order to simplify the testing and future development of the re-weighting.

For running the new code, the following lines serve as an example:

from SimGeneral.MixingModule.pixelDigitizer_cfi import *
process.mix.digitizers.pixel.UseTemplateAgeing = cms.bool(True)

from Configuration.AlCa.GlobalTag import GlobalTag
process.GlobalTag = GlobalTag(process.GlobalTag, '101X_upgrade2018_realistic_Candidate_2018_03_15_16_26_46', '')

The GlobalTag was created for this purpose by @tvami - there are also alternatives for loading the databases available if needed.

Here is a quick note on the concept and plots with tests in the current status: https://desycloud.desy.de/index.php/s/63lG4vCeMNf7XaE

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2018

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22911/4291

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22911/4291/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22911/4291/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2018

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2409196
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2409028
  • DQMHistoTests: Total skipped: 167
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 21 files compared)
  • Checked 107 log files, 9 edm output root files, 27 DQM output files

@perrotta
Copy link
Contributor

+1

  • Modifications to the reco related part here only consist in a (deserved) fix
  • Further improvements to the code sorted out from the discussion: they will be addressed in a subsequent PR (see github issue Check and fix the code for SiPixelTemplate's #20950)
  • Cluster charge reweighting is setup, but not yet enabled in simulation: therefore, no changes are expected on outputs, and nohe is observed in the jenkins tests

@perrotta
Copy link
Contributor

@mdhildreth,@civanch : the changes to this PR since latest sim signature (#22911 (comment)) just consisted in one variable being renamed in a mode consistent way. Please have a look , and sign it if you still think so

@civanch
Copy link
Contributor

civanch commented Apr 16, 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)

@perrotta
Copy link
Contributor

@fabiocos : this PR is fully signed now.
Do you plan to merge it?

@fabiocos
Copy link
Contributor

@perrotta this PR can now go for pre2, in the bunch of changes to digitization to be tested. But I think you raised a good point, the dependency of digitization on local reconstruction looks quite unnatural.
I will not stop this PR, but a reordering of code moving the 2D template in a more neutral place should be considered.

@pmaksim1 I did not fully understand your comment: are you in favour of a reordering or not?

@fabiocos
Copy link
Contributor

@schuetzepaul as far as I understand, the tests above are not really probing the new algorithm, just that the code is by default disabled. SO the real validation of the code is in the results that you show in the slides referenced at the beginning, right?

@schuetzepaul
Copy link
Contributor Author

@fabiocos :

I did not fully understand your comment: are you in favour of a reordering or not?

@pmaksim1 was against moving the code.

as far as I understand, the tests above are not really probing the new algorithm, just that the code is by default disabled. SO the real validation of the code is in the results that you show in the slides referenced at the beginning, right?

Completely correct.

@schuetzepaul
Copy link
Contributor Author

@fabiocos @perrotta
What is the plan for this PR? Do we wait for the reordering of the code (or at least the outcome of the discussion concerning the reordering) or will this be merged without the reordering?

@fabiocos
Copy link
Contributor

fabiocos commented May 6, 2018

@schuetzepaul @pmaksim1 I understand from Petar that the plan discussed in #20950 could be achieved within CMSSW_10_2_X, for this reason I integrate this PR, with the understanding that the pixel group will remove this unwanted dependency in a reasonable timescale.

@fabiocos
Copy link
Contributor

fabiocos commented May 6, 2018

+1

@cmsbuild cmsbuild merged commit f42f238 into cms-sw:master May 6, 2018
@pmaksim1
Copy link
Contributor

pmaksim1 commented May 6, 2018 via email

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

7 participants