PPS full sim specific files only (second part of the splited PR 24420) #24545

mundim · 2018-09-14T14:30:42Z

PPS full simulation code, with new SD for the tracker and timing detectors. It depends on the ongoing PR 24490 (PPSTools and new SimTransport with realistic optics parameterization from TOTEM).

@fabferro @disonjd @jan-kaspar @nminafra @setesami @forthommel @grzanka @nogima

cmsbuild · 2018-09-14T14:31:12Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-09-14T14:33:35Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24545/6422

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-24545/6422/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24545/6422/git-diff.patch | patch -p1

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

cmsbuild · 2018-09-14T14:56:07Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-09-14T14:58:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24545/6425

cmsbuild · 2018-09-14T14:59:07Z

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

It involves the following packages:

Configuration/Geometry
EventFilter/CTPPSRawToDigi
FastSimulation/CTPPSFastTrackingProducer
FastSimulation/CTPPSRecHitProducer
FastSimulation/CTPPSSimHitProducer
Geometry/CMSCommonData
Geometry/VeryForwardData
Geometry/VeryForwardGeometry
IOMC/ParticleGuns
PPSTools/Utilities
SimG4CMS/PPS
SimGeneral/MixingModule
SimPPS/Configuration
SimPPS/PPSPixelDigiProducer
SimPPS/PPSSimTrackProducer
SimPPS/RPDigiProducer
SimTransport/HectorProducer
SimTransport/PPSProtonTransport
SimTransport/TotemRPProtonTransportParametrization

The following packages do not have a category, yet:

PPSTools/Utilities
SimG4CMS/PPS
SimPPS/Configuration
SimPPS/PPSPixelDigiProducer
SimPPS/PPSSimTrackProducer
SimPPS/RPDigiProducer
SimTransport/PPSProtonTransport
SimTransport/TotemRPProtonTransportParametrization
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@perrotta, @efeyazgan, @Dr15Jones, @lveldere, @cvuosalo, @civanch, @perrozzi, @ssekmen, @mdhildreth, @ianna, @cmsbuild, @kpedro88, @slava77, @alberto-sanchez, @qliphy can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @matt-komm, @forthommel, @jan-kaspar, @makortel 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

civanch · 2018-09-15T14:51:05Z

please test

cmsbuild · 2018-09-15T14:51:31Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30431/console Started: 2018/09/15 16:52

cmsbuild · 2018-09-15T14:59:26Z

-1

Tested at: 4c084b3

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

I found follow errors while testing this PR

Failed tests: Build ClangBuild

Build:

I found an error when building:

>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/plugins/CTPPSPixelDigiToRaw.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/plugins/TotemTriggerRawToDigi.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/plugins/CTPPSTotemDigiToRaw.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/plugins/TotemVFATRawToDigi.cc 
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/plugins/CTPPSPixelDigiToRaw.cc: In member function 'virtual void CTPPSPixelDigiToRaw::produce(edm::Event&, const edm::EventSetup&)':
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/plugins/CTPPSPixelDigiToRaw.cc:143:28: error: 'Digis' is not a member of 'CTPPSPixelDataFormatter'
   CTPPSPixelDataFormatter::Digis digis;
                            ^~~~~
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/plugins/CTPPSPixelDigiToRaw.cc:149:5: error: 'digis' was not declared in this scope
     digis[ di->id] = di->data;
     ^~~~~

Clang:

I found a compilation error while trying to compile with clang:
I used this command:
scram b vclean && USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 16 COMPILER='llvm compile'

>> Compiling  /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/SimPPS/PPSPixelDigiProducer/src/RPixHitChargeConverter.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/SimPPS/PPSPixelDigiProducer/src/PPSPixelDigiAnalyzer.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/SimPPS/PPSPixelDigiProducer/src/RPixLinearChargeDivider.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/SimPPS/PPSPixelDigiProducer/src/RPixLinearChargeCollectionDrifter.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/SimPPS/RPDigiProducer/src/RPDigiProducer.cc 
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/src/CTPPSTotemDataFormatter.cc:90:55: error: 'calculateCRC' is a private member of 'VFATFrame'
                for (int i = 11; i >= 1; i--)  crc_fin = VFATFrame::calculateCRC(crc_fin,bufCRC[i]);
                                                                    ^
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_3_X_2018-09-15-1100/src/EventFilter/CTPPSRawToDigi/interface/VFATFrame.h:191:17: note: declared private here
    static word calculateCRC(word crc_in, word dato);
                ^

cmsbuild · 2018-09-15T14:59:28Z

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

cmsbuild · 2018-10-11T15:53:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24545/6805

cmsbuild · 2018-10-11T15:53:34Z

Pull request #24545 was updated. @perrotta, @smuzaffar, @efeyazgan, @Dr15Jones, @lveldere, @cvuosalo, @civanch, @perrozzi, @ssekmen, @mdhildreth, @ianna, @cmsbuild, @mommsen, @franzoni, @emeschi, @slava77, @alberto-sanchez, @qliphy, @fabiocos, @kpedro88, @davidlange6 can you please check and sign again.

perrotta · 2018-10-15T08:47:11Z

@cmsbuild please test

cmsbuild · 2018-10-15T08:47:41Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31025/console Started: 2018/10/15 10:48

cmsbuild · 2018-10-15T09:00:09Z

-1

Tested at: c8c26ac

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

I found follow errors while testing this PR

Failed tests: Build ClangBuild

Build:

I found compilation warning when building: See details on the summary page.

Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 16 COMPILER='llvm compile'

See details on the summary page.

cmsbuild · 2018-10-15T09:00:11Z

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

perrotta · 2018-10-15T09:05:55Z

I've just launched the tests for this PR.

However, I see that (for example) Utilities/PPS/bin/PPSOpticsCalibration.cc is called (more correctly) Utilities/PPS/bin/PPSOpticsCalibrator.cc in #24490. Clearly the two PRs are not kept synchronized (as they should): therefore, they must at least follow some order, and they must be evaluated and merged by strictly following that order.

@mundim please provide us with a map that explains:

Which are the daughter PR of the original PPS Full simulation + PPS proton transport with totem parameterization #24420? Only this one and Splitting into PPSTools + new transport with totem param. and fastsim… #24490
In which order they should be evaluated, and therefore merged?

Once any of them is merged, the other one(s) should get rebased: evaluating those pull requests before they being rebased is a waste of time, since it is evident that they are not kept synchronized and unavoidably they will eventually become different.

Please provide a clear plan as a comment (and also in the description part at the top) in this pull request, in #24490, and in all other possible affected PR: I cannot continue the review till then.

kpedro88 · 2018-10-15T14:41:38Z

The build errors are:

/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_4_X_2018-10-14-2300/src/SimPPS/PPSPixelDigiProducer/interface/PPSPixelDigiProducer.h:69:20: warning: 'CTPPSPixelDigiProducer::beginRun' hides overloaded virtual function [-Woverloaded-virtual]
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_4_X_2018-10-14-2300/src/SimPPS/RPDigiProducer/interface/RPDigiProducer.h:52:20: warning: 'RPDigiProducer::beginRun' hides overloaded virtual function [-Woverloaded-virtual]
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_4_X_2018-10-14-2300/src/SimTransport/PPSProtonTransport/src/HectorTransport.cc:96:18: warning: explicitly assigning value of variable of type 'double' to itself [-Wself-assign]

One of these seems to be the same as a warning seen in #24490.

There is also a code rule violation:

Rule3 : https://raw.githubusercontent.com/cms-sw/cmssw/master/Utilities/ReleaseScripts/python/cmsCodeRules/config.py
Search for "catch(...)" statements in *.cc, *.cxx files

/SimPPS/RPDigiProducer/src/RPDisplacementGenerator.cc
[31]

mundim · 2018-10-17T14:48:07Z

I've just launched the tests for this PR.

However, I see that (for example) Utilities/PPS/bin/PPSOpticsCalibration.cc is called (more correctly) Utilities/PPS/bin/PPSOpticsCalibrator.cc in #24490. Clearly the two PRs are not kept synchronized (as they should): therefore, they must at least follow some order, and they must be evaluated and merged by strictly following that order.

@mundim please provide us with a map that explains:
* Which are the daughter PR of the original #24420? Only this one and #24490

Yes.

* In which order they should be evaluated, and therefore merged?

The PR 24490 should go first.

Once any of them is merged, the other one(s) should get rebased: evaluating those pull requests before they being rebased is a waste of time, since it is evident that they are not kept synchronized and unavoidably they will eventually become different.

Please provide a clear plan as a comment (and also in the description part at the top) in this pull request, in #24490, and in all other possible affected PR: I cannot continue the review till then.

In summary: This PR depends on the PR 24490, so 24490 needs to go first. Then, this one should be rebased. These 2 PRs (24490 and 24545) are the only PR affected.

davidlange6 · 2018-11-15T17:48:42Z

Your PR is unmergeable. Please have a look and possibly rebase it.

dilsonjd · 2018-11-16T11:40:46Z

There is already a new version but as this PR depends on the PR24490 we were waiting for his approval. Do you thing that it is a good strategy?
With which release the rebase must be done?

perrotta · 2018-11-16T13:37:40Z

@dilsonjd : as soon as there will be an IB which includes #24490, then rebase on that IB (or on an IB newer than that)

perrotta · 2018-12-03T14:47:25Z

Periodic check: is there anything new for this PR coming?

dilsonjd · 2018-12-03T16:16:20Z

Still waiting for an IB which includes #25355 (a clean version of #24490).

fabiocos · 2018-12-04T08:36:09Z

I had some comments in #25355 that I would like to see addressed here

@dilsonjd this PR needs to be rebased at this point

civanch · 2018-12-04T09:48:46Z

@mundim , I would suggest to close this PR and start a new one on top of merged PR #25355. The new IB will be available tomorrow. 208 files in the current PR is too much, this would be good to split, I would have 1st part with only new packages alone, in the next implement interaction with the rest of CMSSW. Fimnally we need implement comments from David and Fabio everywhere, provide unit test and/or test simulations, introduce G4Region "PPS".

mundim · 2018-12-05T10:06:12Z

As recommended above, closing this PR, waiting for the approval of PR #25355, when a PR for these packages will be resubmitted.

cmsbuild added this to the CMSSW_10_3_X milestone Sep 14, 2018

cmsbuild added code-checks-pending comparison-pending fastsim-pending generators-pending geometry-pending new-package-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending upgrade-pending labels Sep 14, 2018

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Sep 14, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 14, 2018

cmsbuild added tests-started and removed tests-pending labels Sep 15, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 11, 2018

cmsbuild added tests-started and removed tests-pending labels Oct 15, 2018

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Oct 15, 2018

cmsbuild added new-package-pending and removed core-pending labels Nov 15, 2018

cmsbuild removed the new-package-pending label Dec 4, 2018

mundim closed this Dec 5, 2018

mundim mentioned this pull request Mar 15, 2019

PPS Full simulation #26193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PPS full sim specific files only (second part of the splited PR 24420) #24545

PPS full sim specific files only (second part of the splited PR 24420) #24545

mundim commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

civanch commented Sep 15, 2018

cmsbuild commented Sep 15, 2018 •

edited

Loading

cmsbuild commented Sep 15, 2018

cmsbuild commented Sep 15, 2018

cmsbuild commented Oct 11, 2018

cmsbuild commented Oct 11, 2018

perrotta commented Oct 15, 2018

cmsbuild commented Oct 15, 2018 •

edited

Loading

cmsbuild commented Oct 15, 2018

cmsbuild commented Oct 15, 2018

perrotta commented Oct 15, 2018

kpedro88 commented Oct 15, 2018

mundim commented Oct 17, 2018

davidlange6 commented Nov 15, 2018

dilsonjd commented Nov 16, 2018

perrotta commented Nov 16, 2018

perrotta commented Dec 3, 2018

dilsonjd commented Dec 3, 2018

fabiocos commented Dec 4, 2018

civanch commented Dec 4, 2018

mundim commented Dec 5, 2018

PPS full sim specific files only (second part of the splited PR 24420) #24545

PPS full sim specific files only (second part of the splited PR 24420) #24545

Conversation

mundim commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

cmsbuild commented Sep 14, 2018

civanch commented Sep 15, 2018

cmsbuild commented Sep 15, 2018 • edited Loading

cmsbuild commented Sep 15, 2018

cmsbuild commented Sep 15, 2018

cmsbuild commented Oct 11, 2018

cmsbuild commented Oct 11, 2018

perrotta commented Oct 15, 2018

cmsbuild commented Oct 15, 2018 • edited Loading

cmsbuild commented Oct 15, 2018

cmsbuild commented Oct 15, 2018

perrotta commented Oct 15, 2018

kpedro88 commented Oct 15, 2018

mundim commented Oct 17, 2018

davidlange6 commented Nov 15, 2018

dilsonjd commented Nov 16, 2018

perrotta commented Nov 16, 2018

perrotta commented Dec 3, 2018

dilsonjd commented Dec 3, 2018

fabiocos commented Dec 4, 2018

civanch commented Dec 4, 2018

mundim commented Dec 5, 2018

cmsbuild commented Sep 15, 2018 •

edited

Loading

cmsbuild commented Oct 15, 2018 •

edited

Loading