-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Optical function method for PPS proton transport #29857
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29857/15409
|
A new Pull Request was created by @mundim for master. It involves the following packages: Configuration/Eras @civanch, @silviodonato, @mdhildreth, @cmsbuild, @franzoni, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
-1 Tested at: 9a66943 CMSSW: CMSSW_11_1_X_2020-05-16-1100 I found follow errors while testing this PR Failed tests: Build
I found compilation error when building: >> Package SimPPS/PPSSimTrackProducer built >> Entering Package SimPPS/PPSSimTrackProducer Entering library rule at src/SimPPS/PPSSimTrackProducer/plugins >> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_1_X_2020-05-16-1100/src/SimPPS/PPSSimTrackProducer/plugins/PPSSimTrackProducer.cc /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_1_X_2020-05-16-1100/src/SimPPS/PPSSimTrackProducer/plugins/PPSSimTrackProducer.cc: In constructor 'PPSSimTrackProducer::PPSSimTrackProducer(const edm::ParameterSet&)': /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_1_X_2020-05-16-1100/src/SimPPS/PPSSimTrackProducer/plugins/PPSSimTrackProducer.cc:93:61: error: no matching function for call to 'TotemTransport::TotemTransport(const edm::ParameterSet&, bool&)' theTransporter = new TotemTransport(iConfig, m_verbosity); ^ In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_1_X_2020-05-16-1100/src/SimPPS/PPSSimTrackProducer/plugins/PPSSimTrackProducer.cc:36: /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_1_X_2020-05-16-1100/src/SimTransport/PPSProtonTransport/interface/TotemTransport.h:25:3: note: candidate: 'TotemTransport::TotemTransport(const edm::ParameterSet&)' TotemTransport(const edm::ParameterSet& ps); |
Comparison not run due to Build errors (RelVals and Igprof tests were also skipped) |
Change in the constructor mistakenly not propagated to the calling class. Done. all testes have been done accordingly. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29857/15424
|
Pull request #29857 was updated. @civanch, @silviodonato, @mdhildreth, @cmsbuild, @franzoni, @fabiocos, @davidlange6 can you please check and sign again. |
please test |
The tests are being triggered in jenkins. |
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just realized that yesterday I did not submit these comments :-( . Please implement them in the next PR.
@@ -47,7 +49,7 @@ class PPSSimTrackProducer : public edm::stream::EDProducer<> { | |||
explicit PPSSimTrackProducer(const edm::ParameterSet&); | |||
~PPSSimTrackProducer() override; | |||
|
|||
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions); | |||
//static void fillDescriptions(edm::ConfigurationDescriptions& descriptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please delete the commented lines of code.
m_InTag = iConfig.getParameter<edm::InputTag>("HepMCProductLabel"); | ||
m_InTagToken = consumes<edm::HepMCProduct>(m_InTag); | ||
|
||
m_verbosity = iConfig.getParameter<bool>("Verbosity"); | ||
m_transportMethod = iConfig.getParameter<std::string>("TransportMethod"); | ||
//m_transportMethod = iConfig.getParameter<std::string>("TransportMethod"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
} | ||
|
||
evt = new HepMC::GenEvent(*HepMCEvt->GetEvent()); | ||
|
||
theTransporter->clear(); | ||
//theTransporter->clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
// ------------ method fills 'descriptions' with the allowed parameters for the module ------------ | ||
/* | ||
void PPSSimTrackProducer::fillDescriptions(edm::ConfigurationDescriptions& descriptions) { | ||
//The following says we do not know what parameters are allowed so do no validation | ||
// Please change this to state exactly what you do use, even if it is no parameters | ||
edm::ParameterSetDescription desc; | ||
desc.setUnknown(); | ||
descriptions.addDefault(desc); | ||
} | ||
|
||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps you can delete all this part
+1 |
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 be automatically merged. |
PR description:
Implementation of the new method for proton transport to the PPS region based on Optical Functions from condDB and re-design of PPSProtonTransport and inclusion of ctpps_2021 Modifier (needed on this package).
PR validation:
All standard tests have ben performed with no issue, including code-checks and code-format
The runTheMatrix showed at first run the output:
34 32 29 24 16 4 1 1 1 tests passed, 0 1 2 0 0 0 0 0 0 failed
With the failing tests 136.793, 136.874 and 140.56 all of them after a few (tens) of events processed. Submitting only the failed ones again, with no change, all of them succeeded with output:
3 3 3 2 tests passed, 0 0 0 0 failed
Apparently it was
if this PR is a backport please specify the original PR and why you need to backport that PR:
Before submitting your pull requests, make sure you followed this checklist: