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
Backport of Factory for adding UserHooks to PythiaHadronizer and SUEP decay pythia Hook #36509
Backport of Factory for adding UserHooks to PythiaHadronizer and SUEP decay pythia Hook #36509
Conversation
A new Pull Request was created by @cericeci (Carlos Erice) for CMSSW_10_6_X. It involves the following packages:
@SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: UnitTests RelVals AddOn Unit TestsI found errors in the following unit tests: ---> test TestGeneratorInterfacePythia8InterfaceCompareIdentical had ERRORS ---> test TestGeneratorInterfacePythia8InterfaceCompareExternal had ERRORS ---> test testGeneratorInterfacePythia8InterfaceTP had ERRORS ---> test TestGeneratorInterfacePythia8InterfaceCompareExternalStreams had ERRORS and more ... RelValsThe relvals timed out after 4 hours.
Expand to see more relval errors ...AddOn Tests
|
That's quite unexpected, I didn't got any segfaults when running the local tests. I'll investigate a bit farther on it |
Pull request #36509 was updated. @SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please check and sign again. |
Sorry for the noise, I made a very dumb mistake on the initialization of the added userhooks vector. Can I get the tests redone on this one? |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-98d3cb/21326/summary.html Comparison SummarySummary:
|
+generators |
This pull request is fully signed and it will be integrated in one of the next CMSSW_10_6_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_3_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
backport of #36238 |
The method added to GeneratorInterface/Pythia8Interface/interface/MultiUserHook.h is not from #36238: could you please tell from which development or PR is it coming from? GeneratorInterface/Pythia8Interface/plugins/BuildFile.xml looks rater different here in 10_6 and in the master release: can you explain? The implementation in GeneratorInterface/Pythia8Interface/plugins/Pythia8Hadronizer.cc is rather different with respect to the one in the master: could you please summarize the differences? |
@perrotta thanks for reviewing (and sorry for the amount of your time I'm taking lately on pushing backports) The treatment of collections of UserHooks changed in #29995 (11_2, my understanding is that this was to adapt the code to ran on pythia 3.02, but maybe @agrohsje knows a bit more on the gen side?). Then, the cms class MultiUserHook was replaced by pythia's UserHooksVector. Unfortunately the functionality to loop over the items in the vector doesn't seem to be there for the former so I added a getter to get them. Would it be better if I just define my fCustomHooksVector as an std::vector directly and avoid modifying MultiUserHook.h? I can also check whether with UserHooksVector runs under pythia 2.40 if you want (then the changes would be minimal). For the BuildFile there were several changes between 10_6 and master (#31329, #31405) that were never part of a backport. I only added the minimal changes needed for the plugin to work, but I'm not sure whether anything else should be ported as well (at least scram is not complaining to me). Any suggestions? The differences on Pythia8Hadronizer.cc arise from the first point above, I went backwards from the UserHooksVector syntax to the MultiUserHook one, so to get the same functionality I need to arrange it for the old class syntax. |
@cericeci thank you for your answers. In a backport, maintaining the changes and the number of ported features to the minimum is a welcome plus. Of course it cannot go at the expenses of the functionality, computing performance, and also effort needed to adapt the new addition into the new code. I am not expert enough in this code to judge whether you can, with a limited effort, backport only the new functionalities without the need of bringing together other developments integrated since then. I'll let you and @agrohsje to decide about it. |
@perrotta From my side I'm ok with the current implementation, but if anybody from gen wants me to adapt it farther I have no problem in doing so |
+1 |
Backport of #36238, intended for UL signal production. I did include both changes from the PR for simplicity (the factory and the SUEP plugin itself), but I can take the former out of the code if deemed more appropriate as it is just a backport.
Original PR description
This PR includes two main changes:
PR validation
Unit tests passed.
Tests using a local config file show that the SUEP module properly decays the Higgs boson.
Backporting:
Backport of #36238