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

btag HIP mitigation + ctag bugfix (80X) #15601

Conversation

ferencek
Copy link
Contributor

@ferencek ferencek commented Aug 25, 2016

Backport of #15421 but with the default b-tag track selection unchanged and the HIP mitigation added through an extra set of b-tag discriminators added to slimmedJets.

If needed, the PR will be updates accordingly with what will be decided for the 80X re-reco.

Update (Aug. 29, 2016): This PR is now a verbatim backport of #15421

Update (Aug. 29, 2016): Added backport of #15659

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ferencek (Dinko Ferencek) for CMSSW_8_0_X.

It involves the following packages:

PhysicsTools/PatAlgos

@cmsbuild, @cvuosalo, @slava77, @montjj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @gpetruc, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @JyothsnaKomaragiri this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2016

@cmsbuild 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/14730/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@ferencek
Copy link
Contributor Author

Once this PR gets integrated, all people will need to do in order to get
the looser BTV track selection applied is to re-run b tagging on top of
MiniAOD without any extra switches and this will work for both the old
and new MiniAOD.

One question, though, is whether to backport #15684. It will improve the
AOD vs MiniAOD agreement for the new MiniAOD but will make it worse for
the old MiniAOD.

On 08/30/2016 06:16 PM, Slava Krutelyov wrote:

@ferencek https://github.com/ferencek should this PR also include a
configuration method to allow running looser b-tag track selection on
top of "old" miniAOD which has the default with tight selection ?
Is it actually feasible?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15601 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEqLKWp4xSEnHoFjhAt6GSoCsaQ5dhF0ks5qlFdDgaJpZM4Js2VN.

@slava77
Copy link
Contributor

slava77 commented Aug 31, 2016

On 8/31/16 7:39 AM, Dinko Ferencek wrote:

Once this PR gets integrated, all people will need to do in order to get
the looser BTV track selection applied is to re-run b tagging on top of
MiniAOD without any extra switches and this will work for both the old
and new MiniAOD.

One question, though, is whether to backport #15684. It will improve the
AOD vs MiniAOD agreement for the new MiniAOD but will make it worse for
the old MiniAOD.

Is there a significant performance cost to using
trackMinLayers=0?
Seeing a ROC curve would help.

If it's minimal, then it's better to keep trackMinLayers=0 to be
compatible with the old miniAOD.

On 08/30/2016 06:16 PM, Slava Krutelyov wrote:

@ferencek https://github.com/ferencek should this PR also include a
configuration method to allow running looser b-tag track selection on
top of "old" miniAOD which has the default with tight selection ?
Is it actually feasible?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15601 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEqLKWp4xSEnHoFjhAt6GSoCsaQ5dhF0ks5qlFdDgaJpZM4Js2VN.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15601 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbj-Iy8Ij609suqAnSsmlT27esG-_ks5qlZI_gaJpZM4Js2VN.

@ferencek
Copy link
Contributor Author

@slava77, I agree that keeping trackMinLayers=0 is probably a better solution. This is primarily because of cases where people would prefer to start re-making their ntuples with the BTV HIP mitigation applied on top of the old MiniAOD as opposed to waiting for the re-MiniAOD.

However, this complicates things a bit in terms of the b tagging setups people use and those for which the SF are being provided. And here the ROC curve is not the end of the story because two setups could have very similar ROC curves but different efficiencies for a specific discriminator cut. So we might need a very quick study to see what configuration to keep.

I would also like to hear from @pvmulder, @arizzi, @imarches, @carolinecollard in case they have some suggestions.

@ferencek
Copy link
Contributor Author

ferencek commented Aug 31, 2016

I think that the ROC curves added to the description of #15684 give us the needed answers. It should be fine to stay with trackMinLayers=0 since the performance is similar to the one in AOD and the one with the default trackMinLayers cut using the "new" MiniAOD. However, there could be some small differences in efficiencies/mistag rates for the same discriminator cuts due to small differences in discriminator shapes.

@pvmulder
Copy link
Contributor

pvmulder commented Sep 1, 2016

Seeing these ROC curves and the discriminator shapes makes me think that
it is indeed best to use trackMinLayers=0.

Cheers,
Petra

On 01/09/16 01:33, Dinko Ferencek wrote:

I think that the ROC curves added to the description of #15684
#15684 give us the needed
answers. It should be fine to stay with |trackMinLayers=0| since the
performance is similar to the one in AOD and the one with the default
|trackMinLayers| cut with the "new" MiniAOD. However, there could be
some small differences in efficiencies/mistag rates for the same
discriminator cuts due to small differences in discriminator shapes.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15601 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFLKaV4Ri__wvCJEoXk4RQZr9TKqTv5Dks5qlg9TgaJpZM4Js2VN.


Dr. Petra Van Mulders

Inter-university Institute for High Energies (IIHE),
Vrije Universiteit Brussel (VUB), DNTK-ELEM
Pleinlaan 2, 1050 Brussel, Belgie
+32 2 629 3623
pvmulder@vub.ac.be
pvmulder@cern.ch


@slava77
Copy link
Contributor

slava77 commented Sep 1, 2016

@pvmulder
please comment on the status of the payloads integration for the track/jet probabilities.
This was seen as the holding issue for this PR.

@pvmulder
Copy link
Contributor

pvmulder commented Sep 1, 2016

@slava77 for the calibrations @mmusich is following up, we have given green light and sent the necessary files. At the last PC meeting it was decided that the rereco and tranche 4 MC should happen with the btv mitigation, hence the PR can be merged.

@slava77
Copy link
Contributor

slava77 commented Sep 1, 2016

On 9/1/16 7:50 AM, Petra Van Mulders wrote:

@slava77 https://github.com/slava77 for the calibrations @mmusich
https://github.com/mmusich is following up, we have given green light
and sent the necessary files. At the last PC meeting it was decided that
the rereco and tranche 4 MC should happen with the btv mitigation, hence
the PR can be merged.

Nominally, we should still test the corresponding GT update in 81X.
Given how things develop (payloads still not in 81X),
it looks like we can not yet check the performance of this PR with
retrained probability payloads.
Since this GT update is not operationally required to integrate this PR
(code will run without it, just the prob distributions don't pick up
newly selected tracks), I'll proceed with the final checks of this PR
and will sign off.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15601 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbmjtTS5V9LlcBvKMox7npK2dCPi_ks5qluZRgaJpZM4Js2VN.

@mmusich
Copy link
Contributor

mmusich commented Sep 1, 2016

@slava77 and @pvmulder I am still a bit worried about including the track probability calibrations in GT since has not been derived with the alignments we are proposing both for MC Tranche-IV and data re-reco. Maybe the effect is subleading w.r.t the requirement of btag track mitigation. Please confirm.

@mmusich
Copy link
Contributor

mmusich commented Sep 1, 2016

At any rate, @slava77
for the purpose of testing you can use:
-> run2_mc : 81X_mcRun2_asymptotic_Candidate_2016_09_01_15_54_19
-> run2_data_relval : 81X_dataRun2_relval_Candidate_2016_09_01_15_56_06

@pvmulder
Copy link
Contributor

pvmulder commented Sep 1, 2016

@mmusich the alignment is what it is and should not keep us from having these calibrations, where at least the new track category is defined (which is not the case now). So, it may not be perfect, but at least we will have something working. A new calibration can always be fed afterwards using a recipe that runs on miniAOD (should we see that it is really needed).
On slide 8 and 9 of https://indico.cern.ch/event/565914/contributions/2290892/attachments/1329614/1997547/bloch_btag_JPcalib_31aug16.pdf one can see that the variations with the proposed calibration are reasonable.

@slava77
Copy link
Contributor

slava77 commented Sep 1, 2016

@mmusich
#15601 (comment)
[oddly enough, I still haven't received this note by email]

I see that GT has 3 records changes compared to latest 81X
https://cms-conddb-dev.cern.ch/cmsDbBrowser/diff/Prod/gts/81X_mcRun2_asymptotic_Candidate_2016_08_30_11_31_55/81X_mcRun2_asymptotic_Candidate_2016_09_01_15_54_19
I was going to check an incremental diff with just BTagTrackProbability3DRcd JPcalib_MC80X_v2
I expect that this should make sense still. @pvmulder please correctly me I'm wrong.

@pvmulder
Copy link
Contributor

pvmulder commented Sep 1, 2016

@slava77 that's correct.

@slava77
Copy link
Contributor

slava77 commented Sep 2, 2016

So, based on https://cms-conddb-dev.cern.ch/cmsDbBrowser/diff/Prod/gts/81X_mcRun2_asymptotic_Candidate_2016_08_30_11_31_55/81X_mcRun2_asymptotic_Candidate_2016_09_01_15_54_19
only BTagTrackProbability3DRcd payload is updated.
As a result, only prob3d value has changed in IP tag infos

Here is an incremental diff on top of #15421 on ttbar PU35 (wf 25202)
all_sign745-probgtmanvssign745_ttbarpuwf25202p0c_recocandidateedmptrsrecojettaginforecoiptaginfos_pfimpactparametertaginfos__reco_obj_m_prob3d

I suppose, there will need to be an update for BTagTrackProbability2DRcd as well.

There is a bit of an improvement in IP tag roc (70 events only, can't be too convincing)
wf25202probgtmanv_ip_lvb_roc

And about no change in the combined
wf25202probgtmanv_comb_lvb_roc

@slava77
Copy link
Contributor

slava77 commented Sep 2, 2016

backport of #15421 and #15659

@slava77
Copy link
Contributor

slava77 commented Sep 2, 2016

+1

for #15601 0096fd2

@slava77
Copy link
Contributor

slava77 commented Sep 2, 2016

hold

formal hold to indicate that this PR changes reco and miniAOD products.
Hold can be overwritten or removed when the rereco release is to be built (open for changes in reco/miniAOD products)

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2016

Pull request has been put on hold by @slava77
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Sep 2, 2016
@slava77
Copy link
Contributor

slava77 commented Sep 2, 2016

backport

for some reason my earlier message #15601 (comment) was not seen by the bot to trigger "backport" label

@pvmulder
Copy link
Contributor

pvmulder commented Sep 2, 2016

@slava77 BTagTrackProbability2DRcd is not used, hence no update is expected at this point. Thanks for your support and patience with this PR!

@davidlange6 davidlange6 merged commit ccac392 into cms-sw:CMSSW_8_0_X Sep 7, 2016
@ferencek ferencek deleted the bTagHIPMitigation-PR_from-CMSSW_8_0_17 branch September 12, 2016 00:09
@ferencek ferencek restored the bTagHIPMitigation-PR_from-CMSSW_8_0_17 branch September 12, 2016 00:10
@ferencek ferencek deleted the bTagHIPMitigation-PR_from-CMSSW_8_0_17 branch October 6, 2016 21:40
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

6 participants