APV pulse shape in a separated python file

[bourgatte]

The APV pulse shape is now in a separated file and then stored in a vector whatever its size to easily modify it.
Assuming this is purely technical, nothing should change.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25446/7518

[cmsbuild]

A new Pull Request was created by @bourgatte 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, @pieterdavid, @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

[makortel]

Moving the ~1.5k numbers as doubles to be the configuration is not necessarily the best choice (from the framework point of view). Assuming the plan is to allow changes in the numbers, could they be moved e.g. to EventSetup, or to a separate text file pointed to with cms.FileInPath?

[mmusich]

Moving to ES, with corresponding need to define a record (and eventually a CondFormat) would give indeed the maximum flexibility though probably an overkill for something we might want to change with not more than yearly or multi-yearly frequency. OTOH one might want to store the parameters in an era dependent way (e.g. to emulate pre- and post- VFP APV parameters change). If FileInPath mechanism allows for such flexibility (i.e. multiple input files in the data repo steered via era), it sounds a good option.

[makortel]

@mmusich

If FileInPath mechanism allows for such flexibility (i.e. multiple input files in the data repo steered via era), it sounds a good option.

Sure, it's a configuration parameter type as others, so can be customized with Modifiers (eras). That could then be a good way to proceed (if there indeed is no plan to change these values e.g. within run-dependent MC).

[bourgatte]

I used instead a text file using cms.FileInPath and I will commit to update this pull request.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25446/7573

[cmsbuild]

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

[bourgatte]

I modified what you suggested, moved resolution into pulse shape file and reduce peak&deco duplication in SiLinearChargeDivider. I will commit.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25446/7861

• This PR adds an extra 40KB to repository

• Found files with invalid states:

  • SimGeneral/MixingModule/python/APVShape_default.py:
    • Added: 96a656a
    • Modified: bb5572b, 15df6b7
    • Deleted: 82e8e6c
  • SimGeneral/MixingModule/test/APVShapePeak_default.txt:
    • Added: 82e8e6c
    • Modified: 64ca711
    • Deleted: aa5d83f
  • SimGeneral/MixingModule/test/APVShapeDeco_default.txt:
    • Added: 82e8e6c
    • Modified: 64ca711
    • Deleted: aa5d83f

[cmsbuild]

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

[fabiocos]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32487/console Started: 2019/01/09 10:22

[cmsbuild]

+1
Tested at: aa5d83f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25446/32487/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 2 differences found in the comparisons
• DQMHistoTests: Total files compared: 33
• DQMHistoTests: Total histograms compared: 3153717
• DQMHistoTests: Total failures: 15
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3153498
• 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

[pieterdavid]

The differences are in the 150.0 workflow, so I suppose they are due to #25570 (if there were anything wrong with the changes in this PR we should see differences in all workflows that include digitization)

[civanch]

+1

[cmsbuild]

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)

[mmusich]

@fabiocos, may I kindly suggest you to review this PR and eventually merge it soon?
The code changes introduced here should not be changing physics at all, i.e. are of purely technical nature.
On the other hand, we have some other changes which rely on top of these ones which are expected to change simulation and we'd really appreciate to get the new PR in by the time of 10_5_0.
Please let us know,
@pieterdavid @echabert

[fabiocos]

@mmusich this looks ok to me to enter in next IB, my comment was taken into account

[fabiocos]

+1