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

Adding Pythia8 capability to TopDecaySubset #7577

Merged
merged 1 commit into from Feb 10, 2015

Conversation

kreczko
Copy link
Contributor

@kreczko kreczko commented Feb 5, 2015

This pull request is for the TopQuarkAnalysis package and enables TopDecaySubset to process the new Pythia8 genparticles.
Since it was requested to preserve the previous functionality, the new code is only used if the new parameter, runMode is set to 'Run2'.

The changes were rebased upon the current master (CMSSW_7_4_X) but also work on CMSSW_7_0_9, CMSSW_7_2_X and CMSSW_7_3_X.
The code has been tested by two independent analysis groups on Spring14 (AOD) and Phys14 (miniAOD) samples.

Since this is my first CMSSW contribution since the github migration, please let me know if I am missing any steps in the procedure.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2015

A new Pull Request was created by @kreczko (Luke Kreczko) for CMSSW_7_4_X.

Adding Pythia8 capability to TopDecaySubset

It involves the following packages:

TopQuarkAnalysis/Configuration
TopQuarkAnalysis/TopEventProducers

@cmsbuild, @vadler, @nclopezo, @monttj can you please review it and eventually sign? Thanks.
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.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2015

@@ -43,12 +47,34 @@ class TopDecaySubset : public edm::EDProducer {
private:
/// find top quarks in list of input particles
std::vector<const reco::GenParticle*> findTops(const reco::GenParticleCollection& parts);
/// find primal top quarks (top quarks from the hard interaction)
/// for Pythia6 this is identical to findDecayingTops
std::vector<const reco::GenParticle*> findPrimalTops(const reco::GenParticleCollection& parts);
Copy link
Contributor

Choose a reason for hiding this comment

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

@kreczko
Hello,
If this findPrimalTops is identical to findTops for pythia6, we could simply replace the implementation of findTops with new one. In this way, it can be transparent for any analyzers who are using this function for both Run 1 and Run 2?
Maybe I missed something here.

Regards,
Taejeong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monttj
Hi,
While the functionality is identical I was told by the TOP PAG conveners not to change run1 code but instead build in switches to enable the new code on demand. I am all in favour in reducing code on the one hand, but on the other I would not replace the code without a unit test verifying its functionality.

Maybe I can add such tests and talk to the conveners. Do you know of any good unit test examples where GenParticleCollections are used? I am not sure how I would construct a mock genparticle collection.

Cheers,
Luke

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Luke,

I am not sure about the unit test for this package.
But you already communicated with TOP PAG conveners and agreed each other,
and this does not affect old functionality,
so I don't see any problem to have this necessary new features for Run 2.

Thank you for the development.

Regards,
Taejeong

@monttj
Copy link
Contributor

monttj commented Feb 9, 2015

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2015

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

@davidlange6
Copy link
Contributor

+1

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

4 participants