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

80x re-miniAOD+Boosted Tau #13100

Merged
merged 26 commits into from Feb 5, 2016

Conversation

isobelojalvo
Copy link
Contributor

Adding in boosted taus to the miniAOD sequence. For a full presentation and summary of changes please see here:
https://indico.cern.ch/event/484473/session/4/contribution/21/attachments/1214722/1773311/Camilla_BoostedTausCode_140116.pdf

This pull request goes on top of 13060:
#13060
And, due to the integration of the code, is dependent on 13060

This should affect in no way the non-boosted tau reconstruction.

cgalloni and others added 12 commits January 25, 2016 21:50
- added bendCorrMass to PFTauSpecific data-format so that it can be accessed from pat::Taus
…Taus_forPull_v800pre4' of git://github.com/cgalloni/cmssw into cgalloni-80ReMiniAODwBoostedTaus_forPull_v800pre4 git merge --no-ff cgalloni-80ReMiniAODwBoostedTaus_forPull_v800pre4 git push

origin 80xReMiniAOD-BoostedTau# Please enter a commit message to explain why this merge is necessary,
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @isobelojalvo for CMSSW_8_0_X.

It involves the following packages:

DataFormats/PatCandidates
PhysicsTools/PatAlgos
RecoJets/JetAlgorithms
RecoJets/JetProducers
RecoTauTag/Configuration
Validation/RecoTau

@cvuosalo, @monttj, @cmsbuild, @deguio, @slava77, @vadler, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @yslai, @schoef, @ferencek, @gpetruc, @mariadalfonso, @pvmulder 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

@deguio
Copy link
Contributor

deguio commented Jan 28, 2016

please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

Pull request #13100 was updated. @cvuosalo, @monttj, @cmsbuild, @deguio, @slava77, @vadler, @vanbesien, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2016

Pull request #13100 was updated. @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 4, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2016

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

@isobelojalvo
Copy link
Contributor Author

@slava77 @cvuosalo thinking a bit more, this modification of discriminators here is actually likely to cause issues at the Pattuple step. Our other option is to create a boostedTau eventContent cfi. Although, it has always been the intention to keep reco and boosted tau objects similar in content.

@slava77
Copy link
Contributor

slava77 commented Feb 4, 2016

On 2/4/16 10:28 AM, isobelojalvo wrote:

@slava77 https://github.com/slava77 @cvuosalo
https://github.com/cvuosalo thinking a bit more, this modification of
discriminators here is actually likely to cause issues at the Pattuple
step. Our other option is to create a boostedTau eventContent cfi.
Although, it has always been the intention to keep reco and boosted tau
objects similar in content.

Isobel,

please clarify what you mean.

My understanding is that
pattuple is supposedly a separate cmsRun to be executed on AOD inputs.
AOD files are not supposed to have MVA3_DMwoLT objects.
Hence, keep statements for these MVA3_DMwoLT are removed from the
general event content.
(skype is a possible another alternative way to clarify)


Reply to this email directly or view it on GitHub
#13100 (comment).

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2016

@isobelojalvo
Copy link
Contributor Author

Hi @slava77 , @cgalloni has convinced me that what I said above is incorrect and we should be fine. Thanks though!

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 4, 2016

With this latest change, the MVA3*DMwoLT discriminators no longer appear, and there is no addition to RECO event content.
However, CPU timing does not appear to improve. The PR seems to increase time per event by up to 4%.
A test of 200 events with workflow 1313.0_QCD_Pt_3000_3500_13 against baseline CMSSW_8_0_X_2016-02-01-2300 gives the following results:

Summary for 200 events
Baseline
Max VSIZ 2184.24 on evt 34 ; max RSS 1825.09 on evt 197
Time av 13.0077 s/evt   max 50.0127 s on evt 155
M1 Time av 12.8769 s/evt   max 50.0127 s on evt 155
M8 Time av 12.6702 s/evt   max 50.0127 s on evt 155

PR 13100
Max VSIZ 2189.24 on evt 34 ; max RSS 1790.3 on evt 155
Time av 13.5929 s/evt   max 53.2566 s on evt 155
M1 Time av 13.4623 s/evt   max 53.2566 s on evt 155
M8 Time av 13.2484 s/evt   max 53.2566 s on evt 155

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 4, 2016

More timing measurements from the 1313 workflow:

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.31%         0.00 ms/ev ->        39.17 ms/ev combinatoricRecoTausBoosted
       added      +0.29%         0.00 ms/ev ->        36.88 ms/ev hpsPFTauPrimaryVertexProducerBoosted
       added      +0.20%         0.00 ms/ev ->        25.36 ms/ev ca8PFJetsCHSprunedForBoostedTaus
       added      +0.10%         0.00 ms/ev ->        12.90 ms/ev recoTauAK4PFJets08RegionBoosted
   +0.058156      +0.07%       159.55 ms/ev ->       169.11 ms/ev tobTecStepTrackCandidates
  ---------- ------------     --------                  ----       ------------
Job total:  12.8304 s/ev ==> 13.402 s/ev (+4.7%)

The same excluding the first 1 event
  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.31%         0.00 ms/ev ->        38.91 ms/ev combinatoricRecoTausBoosted
       added      +0.29%         0.00 ms/ev ->        36.71 ms/ev hpsPFTauPrimaryVertexProducerBoosted
       added      +0.20%         0.00 ms/ev ->        25.18 ms/ev ca8PFJetsCHSprunedForBoostedTaus
       added      +0.10%         0.00 ms/ev ->        12.78 ms/ev recoTauAK4PFJets08RegionBoosted
   +0.058808      +0.07%       154.94 ms/ev ->       164.33 ms/ev tobTecStepTrackCandidates
  ---------- ------------     --------                  ----       ------------
Job total:  12.5532 s/ev ==> 13.1144 s/ev (+4.5%)

Note that the entry for tobTecStepTrackCandidates shows the biggest change for an existing module, to indicate that the change in timing may be due more to new modules than to existing ones and to give an idea of the measurement uncertainty.

@cgalloni
Copy link
Contributor

cgalloni commented Feb 4, 2016

Dear Carl,
this tobTecStepTrackCandidates module I cannot find it in my local config dump.
It wasn't in the list of the most consuming modules before, so now I'm a bit surprised.
Could you help me understand where is this called?

Thanks a lot

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 4, 2016

unhold

@cmsbuild cmsbuild removed the hold label Feb 4, 2016
@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 4, 2016

@cgalloni: Sorry, that module is only included as reference, as the biggest change of an existing module. You should not worry about it.
I''ll edit my posting accordingly.

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 4, 2016

+1

For #13100 a9e01a4

Second approval after minor clean-up of unneeded discriminators. New Jenkins results are still OK, and a new test of workflow 1313.0_QCD_Pt_3000_3500_13 with 200 events against baseline CMSSW_8_0_X_2016-02-01-2300 still shows no significant differences. As discussed above, RECO event content is no longer altered by this PR. There are still performance and multi-threading issues that will need to be addressed in a later PR.

davidlange6 added a commit that referenced this pull request Feb 5, 2016
@davidlange6 davidlange6 merged commit c089cf7 into cms-sw:CMSSW_8_0_X Feb 5, 2016
@roger-wolf roger-wolf deleted the 80xReMiniAOD-BoostedTau branch March 24, 2016 22:22
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

9 participants