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 generator parameter scan functionality to 71x #19443
Conversation
A new Pull Request was created by @bendavid (Josh Bendavid) for CMSSW_7_1_X. It involves the following packages: GeneratorInterface/Configuration @cmsbuild, @govoni, @perrozzi, @thuer, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@intrepid42 in order to bring things into sync, I effectively reverted your pull requests related to recent powheg veto hooks and then re-backported them from master. Please check that the end result makes sense. |
please test |
The tests are being triggered in jenkins. |
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.
Ok, I have just 2 small comments
else if (event[iDau].id() < 0) ida = iDau; | ||
if (abs(event[iDau].id()) == 21) idg = iDau; | ||
else if (abs(event[iDau].id()) > 0) idq = iDau; | ||
else if (abs(event[iDau].id()) < 0) ida = iDau; |
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.
This introduces a bug (abs() is always > 0)
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.
Ok thanks, indeed I missed a commit when restoring your changes, will update the pull request accordingly.
@@ -1,68 +0,0 @@ | |||
import FWCore.ParameterSet.Config as cms |
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 add back the example config.
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.
Will do.
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
This still needs updates before it can be merged, given the comments from @intrepid42 |
Comparison is ready Comparison Summary:
|
please test |
The tests are being triggered in jenkins. |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
1 similar comment
from what I can see and I learnt by discussing with Josh, things look ok and the feature will be active only when explicitly set ON, approving from the GEN side |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_9_3_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
One caveat which I previously forgot about. This might cause problems for future 2015 MC requests with DIGI-RECO in 76x unless the corresponding data formats are added in 76x as well. (Everything is already present in 80x and above, so no problem for 2016 MC) |
I have not seen a single request for 2015 MC in the last 18 months :-) |
looks like its been at least 9 months since a 2015 request. Lets agree that 2015 GEN-SIM requests are now frozen to use the existing 71x releases. |
+1 |
agreed, thanks a lot |
Also brings Pythia8Interface ~in sync with master in general for easier backporting in the future.
@mkirsano