Skip to content
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

Changes to PartonShowerBsHepMCFilter and LHEGenericFilter #11190

Merged
merged 3 commits into from Sep 21, 2015

Conversation

perrozzi
Copy link
Contributor

@perrozzi perrozzi commented Sep 8, 2015

No description provided.

@bendavid
Copy link
Contributor

bendavid commented Sep 8, 2015

+1

@cmsbuild cmsbuild added this to the Next CMSSW_7_6_X milestone Sep 8, 2015
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 8, 2015

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs or unless it breaks tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 8, 2015

A new Pull Request was created by @perrozzi for CMSSW_7_6_X.

port PR 11149 to 76X

It involves the following packages:

GeneratorInterface/Core
GeneratorInterface/GenFilters

@cmsbuild can you please review it and eventually sign? Thanks.
@mkirsano this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@davidlange6
Copy link
Contributor

@perrozzi - please change the title of this PR to briefly describe the proposed changes for the release notes.

@perrozzi perrozzi changed the title port PR 11149 to 76X Changes to PartonShowerBsHepMCFilter and LHEGenericFilter Sep 10, 2015
@perrozzi
Copy link
Contributor Author

sorry, it is done now. let me know if you want me to add more details

@davidlange6
Copy link
Contributor

perfect, thanks!

On Sep 10, 2015, at 10:39 AM, perrozzi notifications@github.com wrote:

sorry, it is done now. let me know if you want me to add more details


Reply to this email directly or view it on GitHub.

@davidlange6
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs or unless it breaks tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@@ -49,8 +49,10 @@ class LHEGenericFilter : public edm::EDFilter {

edm::EDGetTokenT<LHEEventProduct> src_;
int numRequired_; // number of particles required to pass filter
bool acceptMore_; // if true (default), accept numRequired or more.
// if false, accept events with exactly equal to numRequired.
std::string acceptLogic_; // LT meaning <
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you store this as an enum both to ensure that its always valid and to make the string comparison only once rather than many times?
@perrozzi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, I am not sure I understand what/how to do it.
to reduce the number of string comparisons to 1 is easy, but what do you mean with "always valid"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean something like this
perrozzi@a4863f0

@perrozzi
Copy link
Contributor Author

hi, may I kindly ask you to check please? these PR is becoming a bit urgent, as it needs also to be backported to 71X to produce some MC samples. thanks a lot

@cmsbuild
Copy link
Contributor

Pull request #11190 was updated. @vciulli, @covarell, @thuer, @cmsbuild, @bendavid, @govoni can you please check and sign again.

@covarell
Copy link
Contributor

+1

@bendavid
Copy link
Contributor

In the current code, whichlogic would be uninitialized if an invalid string was given.
Better to add an else LogError at the end to catch an invalid string.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs or unless it breaks tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@perrozzi
Copy link
Contributor Author

ok thanks, please check again

@bendavid
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

Pull request #11190 was updated. @cmsbuild can you please check and sign again.

@bendavid
Copy link
Contributor

+1

davidlange6 added a commit that referenced this pull request Sep 21, 2015
Changes to PartonShowerBsHepMCFilter and LHEGenericFilter
@davidlange6 davidlange6 merged commit c9e8b10 into cms-sw:CMSSW_7_6_X Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants