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

Bugfix for pseudotop bjet constituents #15079

Closed

Conversation

mseidel42
Copy link
Contributor

Bugfix for PseudoTopProducer adding random particles to b jet constituents, using negative indices for B hadrons now

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @intrepid42 (Markus Seidel) for CMSSW_7_6_X.

It involves the following packages:

TopQuarkAnalysis/TopEventProducers

@cmsbuild, @davidlange6 can you please review it and eventually sign? Thanks.
@kreczko this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

for ( size_t i=0, n=genParticleHandle->size(); i<n; ++i )
{
const reco::Candidate& p = genParticleHandle->at(i);
const int status = p.status();
if ( status == 1 ) continue;

// Collect B-hadrons, to be used in b tagging
if ( isBHadron(&p) ) bHadronIdxs.insert(i);
if ( isBHadron(&p) ) bHadronIdxs.insert(-i);
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be a matter of convention, but how about store the index as is bHadronIdxs.insert(i); here and set_user_index(-index-1)?

  • subtracting by another 1 to make the user index to be negative-definite to be 100% safe from overlapping.

@mseidel42
Copy link
Contributor Author

Hi John,

the user_index and the index stored in bHadronIdxs need to be both negative, otherwise bHadronIdxs.find(index) is successful for non-bHadron indices as well.
Adding -1 is a good idea to prevent problems with index=0 but then we need to use genParticleHandle->at(abs(index+1)), right?

Cheers,
Markus

@jhgoh
Copy link
Contributor

jhgoh commented Jul 11, 2016

@intrepid42, I agree. its clear. and correct for the 2nd item as well.

@cmsbuild
Copy link
Contributor

Pull request #15079 was updated. @cmsbuild, @davidlange6 can you please check and sign again.

@mseidel42
Copy link
Contributor Author

mseidel42 commented Aug 17, 2016

Plot of deltaR(jet, jet consituent) illustrating the bug. We should expect a steep drop-off around the jet radius (0.4), which clearly does not happen for b jets.

pseudotopproducerbug

The slightly larger tail in non-b vs reco is due to the rapidity-clustering catching a few more low-momentum particles that are not present in reco.

@mseidel42
Copy link
Contributor Author

mseidel42 commented Aug 18, 2016

Addendum: Buggy code seems to generate a mistagged jet in ~40% of the events.

@davidlange6
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 25, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/14726/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@kreczko
Copy link
Contributor

kreczko commented Sep 28, 2016

Our analysis group just realised this was never merged.
What is the procedure to get this into CMSSW? Current master is 81X which will soon change to 82X I assume.

@intrepid42 would you be prepared to create new PRs?

@monttj
Copy link
Contributor

monttj commented Oct 13, 2016

@cmsbuild please test
Indeed, this PR needs to be merged.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 13, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15704/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@monttj
Copy link
Contributor

monttj commented Oct 14, 2016

+1

@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. @slava77, @davidlange6, @smuzaffar

@monttj
Copy link
Contributor

monttj commented Oct 14, 2016

@intrepid42
Right, did we already have the same PR for 80X and 81X?

@mseidel42
Copy link
Contributor Author

@monttj I am not sure...

@kreczko @msavitskyi Did one of you create pull requests for 80X and 81X?

@kreczko
Copy link
Contributor

kreczko commented Oct 17, 2016

Not me.

@msavitskyi
Copy link
Contributor

I created this pull-request for bugfix in PSeudoTop for 81X releases, which is needed starting from CMSSW_8_1_0_pre3 releases: #16190

As far as I understand, #15079 is referenced there.

@mseidel42
Copy link
Contributor Author

Ok, #16190 actually fixes another bug, not this one...

I will create new PRs then.

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

7 participants