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 hooks to tau sequence to read from DB #4585

Merged
merged 4 commits into from Jul 14, 2014

Conversation

jpavel
Copy link
Contributor

@jpavel jpavel commented Jul 9, 2014

  • restoring PhysicsTFormulaPayload and PhysicsTGraphPayload plugins deleted in earlier release
  • adding interfaces to tau MVA discriminators to be able to read also from the sqlite
  • adding a master python switch that makes all relevant discriminators read from sqlite instead of local root file

These pre-requsites are necessary for the tau payloads to be moved to CondDB. This PR does not change default RECO at all, only adds possibility to run with different input that is turned off by default.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2014

A new Pull Request was created by @jpavel (Pavel Jez) for CMSSW_7_2_X.

Adding hooks to tau sequence to read from DB

It involves the following packages:

CondCore/PhysicsToolsPlugins
RecoTauTag/Configuration
RecoTauTag/RecoTau

@apfeiffer1, @nclopezo, @cmsbuild, @StoyanStoynev, @slava77, @ggovi, @Degano can you please review it and eventually sign? Thanks.
@ghellwig 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.

@jpavel
Copy link
Contributor Author

jpavel commented Jul 10, 2014

just out of curiosity, any chance this one makes it to 720_pre2? It would allow us to upload payloads to the database. Otherwise we will need to wait for the next pre-release

@StoyanStoynev
Copy link
Contributor

I am to start extended tests for it and look at the code today but we need the jenkins tests too.

@jpavel
Copy link
Contributor Author

jpavel commented Jul 10, 2014

thanks Stoyan!

@slava77
Copy link
Contributor

slava77 commented Jul 10, 2014

pre2 is out already (not announced yet)

@davidlange6
Copy link
Contributor

Actually, it is not out yet given troubles with various things

On Jul 10, 2014, at 10:12 AM, Slava Krutelyov notifications@github.com
wrote:

pre2 is out already (not announced yet)


Reply to this email directly or view it on GitHub.

@cmsbuild
Copy link
Contributor

-1
Tested at: 247ff14
When I ran the RelVals I found an error in the following worklfows:
1000.0 step3

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step3_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log
----- Begin Fatal Exception 10-Jul-2014 10:15:16 CEST-----------------------
An exception of category 'ProductNotFound' occurred while
   [0] Processing run: 165121 lumi: 62 event: 23623846
   [1] Running path 'tcPath'
   [2] Calling event method for module CandViewSelector/'tcMETSelector'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for a container with elements of type: reco::Candidate
Looking for module label: tcMet
Looking for productInstanceName: 
   Additional Info:
      [a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.
----- End Fatal Exception -------------------------------------------------

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4585/308/summary.html

@jpavel
Copy link
Contributor Author

jpavel commented Jul 10, 2014

Isn't it problem of a relval? This PR was running fine in CMSSW_7_2_X_2014-07-09-0200

@ghost
Copy link

ghost commented Jul 10, 2014

Yes, it is.

@StoyanStoynev
Copy link
Contributor

Then can we have the jenkins/comparisons to a previous IB (like CMSSW_7_2_X_2014-07-09-0200 above or *-1400)? As Slava pointed to me CMSSW_7_2_X_2014-07-10-0200 has build errors so it should not be used for comparisons anyway: https://cmssdt.cern.ch/SDT/jenkins-artifacts/summary-merged-prs/merged_prs.html

@StoyanStoynev
Copy link
Contributor

@jpavel I see somedifferences in MVA electron rejection variables:
isopfcand
sigpfcand.
Differences come at high eta:
elrej_eta
Seen in hpsPFTauDiscriminationByMVA5*ElectronRejection with * -> Loose, Medium, Tight, VTight.
The figures are from wf 202 (ttbar with PU) and wf 38 (QCD) show consistent results. In wf 38 though differences are also found in hpsPFTauDiscriminationByMediumIsolationMVA3oldDMwLT hpsPFTauDiscriminationByVLooseIsolationMVA3oldDMwLT.
What would you say about these?

@StoyanStoynev
Copy link
Contributor

a small correction : hpsPFTauDiscriminationByVLooseIsolationMVA3oldDMwLT -> hpsPFTauDiscriminationByVLooseIsolationMVA3newDMwLT

@@ -564,8 +584,7 @@
discriminator = cms.InputTag('hpsPFTauDiscriminationByDecayModeFindingNewDMs'),
selectionCut = cms.double(0.5)
)
),
cut = cms.string("pt > 18.0 & abs(eta) < 2.4")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected effect of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should reconstruct the primary vertex for more taus, probably affecting the lifetime discriminators. I think we can revert this change for the time being.

@jpavel
Copy link
Contributor Author

jpavel commented Jul 11, 2014

Thanks for having a look - those changes are indeed not expected. I was the only author, so let me check the pieces written by Christian

@jpavel
Copy link
Contributor Author

jpavel commented Jul 11, 2014

So I reverted the only piece of code which should have changed something - the rest is cosmetics.

@cmsbuild
Copy link
Contributor

Pull request #4585 was updated. @ggovi, @cmsbuild, @apfeiffer1, @Degano, @nclopezo can you please check and sign again.

@StoyanStoynev
Copy link
Contributor

Sounds good. What is left looks trivial but I'll make the complete tests
given there is something we didn't understand (moreover jenkins will not run before 72X is fixed as I understood; my tests will also suffer a bit but not in a crucial way).

@StoyanStoynev
Copy link
Contributor

Actually my signature is not needed anymore. Just for completeness - no differences are observed with the updated PR in the extended matrix tests (and none expected with the trivial now code updates).

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@jpavel
Copy link
Contributor Author

jpavel commented Jul 14, 2014

@ggovi, @apfeiffer1, @Degano, @nclopezo can you please have a look and sign? It will be VERY appreciated if we can get this update to 720_pre2 and so to CondDB SW

@apfeiffer1
Copy link
Contributor

+1
sorry for being late with this ... :(

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

ktf added a commit that referenced this pull request Jul 14, 2014
Adding hooks to tau sequence to read from DB
@ktf ktf merged commit 2d6846c into cms-sw:CMSSW_7_2_X Jul 14, 2014
@jpavel jpavel deleted the 72x_taus_SQlite branch July 15, 2014 11:57
@jpavel jpavel mentioned this pull request Jul 16, 2014
jpavel pushed a commit to jpavel/cmssw that referenced this pull request Aug 21, 2014
Adding hooks to tau sequence to read from DB
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