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

High pt taus: TauID for 2014 #1817

Merged
merged 46 commits into from
Jan 27, 2014
Merged

Conversation

jpavel
Copy link
Contributor

@jpavel jpavel commented Dec 13, 2013

Large upgrade of PFTau sequence. Main changes are

  1. Change in algorithm logic
  2. Change in data format (new members plus shift from Ref to Ptr)
  3. Addition of tau lifetime information
  4. Addition of boosted tau subjet techniques
  5. Many new discriminants

jpavel and others added 26 commits December 10, 2013 23:45
…cer.cc

- run CMSBoostedTauSeedingAlgorithm regardless of pruning/filtering being used in addition
- added protection against pathological cases to BoostedTauSeedsProducer.cc
- minor bug-fix: added missing initialization of useCMSBoostedTauSeedingAlgorithm_ flag
(forward-port of commits
   veelken@6fac048
   veelken@045b688
 from CMSSW_6_2_X branch)
- switched to getByToken and consumes interface
- improved include guard and removed cvs specific labels from CMSBoostedTauSeedingAlgorithm.h
…s/PFTauElementsOperators in order to resume 'old' HLT path
…om-CMSSW_7_0_X_2013-12-13-1400

Updating with latest developments
@cmsbuild
Copy link
Contributor

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

High pt taus: TauID for 2014

It involves the following packages:

DataFormats/PatCandidates
RecoTauTag/TauTagTools
JetMETCorrections/TauJet
CommonTools/ParticleFlow
RecoTauTag/Configuration
RecoTauTag/ImpactParameter
RecoTauTag/RecoTau
DataFormats/JetReco
DataFormats/TauReco
PhysicsTools/PatAlgos
RecoJets/JetAlgorithms
PhysicsTools/IsolationAlgos
Validation/RecoTau
RecoJets/JetProducers
HLTriggerOffline/Tau
RecoParticleFlow/Benchmark

@nclopezo, @danduggan, @rovere, @cmsbuild, @anton-a, @thspeer, @deguio, @slava77, @vadler, @eliasron can you please review it and eventually sign? Thanks.
@TaiSakuma 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.
@ktf you are the release manager for this.

@cmsbuild
Copy link
Contributor

-1
I ran the usual tests and I found errors in the following unit tests:

---> test runtestPhysicsToolsPatAlgos had ERRORS
---> test runtestTqafTopEventProducers had ERRORS
---> test runtestTqafTopEventSelection had ERRORS
---> test runtestTqafExamples had ERRORS

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

@rappoccio rappoccio mentioned this pull request Dec 15, 2013
…inators will run in CMSSW_7_0_x per default
@ktf
Copy link
Contributor

ktf commented Jan 20, 2014

For the record: with @davidlange6 we have decided to go ahead with pre12
(currently uploading). This will end up in some pre13 or in the final
CMSSW_7_0_0. To be discussed tomorrow.

On 20 Jan 2014, at 23:19, slava77 wrote:

OK.
Just to summarize for now: the main issues with the code (performance
and quality) appear to be addressed.
I have started integration tests and will hopefully be able to sign
off tomorrow (AM), assuming nothing bad turns up.


Reply to this email directly or view it on GitHub:
#1817 (comment)

@monicava
Copy link

And thanks to @slava77 for his heroic effort!

@jpavel
Copy link
Contributor Author

jpavel commented Jan 20, 2014

agree with Monica - thank you Slava very much for your effort, patience and help. I will try to keep your coding advice in mind for future developments

@slava77
Copy link
Contributor

slava77 commented Jan 21, 2014

Notes on the latest changes (5197cd5..3a08960)

New warnings showed up.
It may be from the last changes in the PV producer, could it be that the same track ends up on the list twice?

%MSG-w TwoTrackMinimumDistance:   PFTauPrimaryVertexProducer:hpsPFTauPrimaryVertexProducer  21-Jan-2014 00:36:06 CET Run: 1 Event: 11272
Computation HelixHelix::CAIR failed.
%MSG
%MSG-w TwoTrackMinimumDistance:   PFTauPrimaryVertexProducer:hpsPFTauPrimaryVertexProducer  21-Jan-2014 00:36:06 CET Run: 1 Event: 11272
comparing track with itself!

It looks like there are no changes in the tau content. So, fixing the above is not urgent, but please keep them on your todo list.

@slava77
Copy link
Contributor

slava77 commented Jan 21, 2014

Timing went down by about x2 - 2.5.
One good example, hpsPFTauPrimaryVertexProducer time reduced by x8 in 40PU25ns sample.

@slava77
Copy link
Contributor

slava77 commented Jan 21, 2014

+1

tested 3a08960 in CMSSW_7_0_X_2014-01-20-1400 (sign298).
Based on the review above.

@slava77
Copy link
Contributor

slava77 commented Jan 21, 2014

@vadler @deguio
Volker and Federico, could you please check this PR and sign it again.

@vadler
Copy link

vadler commented Jan 21, 2014

+1
The last update does not touch AT.

@deguio
Copy link
Contributor

deguio commented Jan 21, 2014

+1
I have only one question to @jpavel and @monicava
Do you really need a web page template to be included in release? Why don't you use the GUI instead?

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_0_X IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

@jpavel
Copy link
Contributor Author

jpavel commented Jan 21, 2014

To include the web page template was suggestion/request from the tau POG validation team who use it to store and publish results of the validation for a long time already. Until now they had to copy these scripts from some semi-private areas so inclusion in the release makes it easier to maintain and monitor. Nevertheless, is there a Twiki page describing how to use the validation GUI, which we can forward to our validators so they can learn how use it? This way we can remove the web templates from the release in the future

@monicava
Copy link

Hi,
Which template are you referring to?
Monica

On 21 Jan 2014, at 16:23, deguio wrote:

+1
I have only one question to @jpavelhttps://github.com/jpavel and @monicavahttps://github.com/monicava
Do you really need a web page template to be included in release? Why don't you use the GUI instead?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1817#issuecomment-32894658.

@deguio
Copy link
Contributor

deguio commented Jan 21, 2014

@jpavel
the histograms should be already available in the GUI.
in the following twiki the instructions on how to develop layouts and render plugins and test them using a temporary GUI are reported:
https://twiki.cern.ch/twiki/bin/viewauth/CMS/DQMTest#Installing_the_GUI_Server

I hope that helps. we are available if you have questions.
thanks,
F.

@monicava
Copy link

The time-scale for updating DQM code depends on the release cycle
which can be quite slow, but I recall that also including new layouts and
render plugins took months to happen.
Hast the situation improved on that front?

Thanks,
Monica

On 21 Jan 2014, at 16:45, deguio wrote:

@jpavelhttps://github.com/jpavel
the histograms should be already available in the GUI.
in the following twiki the instructions on how to develop layouts and render plugins and test them using a temporary GUI are reported:
https://twiki.cern.ch/twiki/bin/viewauth/CMS/DQMTest#Installing_the_GUI_Server

I hope that helps. we are available if you have questions.
thanks,
F.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1817#issuecomment-32896963.

@deguio
Copy link
Contributor

deguio commented Jan 21, 2014

ciao @monicava
you are right. the updates to the DQM code follow the release cycle. layouts and render plugins follows the cmsweb updates instead and they don't depend on us actually. But in standard conditions I don't think that frequent updates are needed. This is the same for all the packages. Am I wrong?
anyway the pull is fine. I was wondering if moving to a GUI based validation is something planned.

thanks,
F.

@monicava
Copy link

Ciao @deguio!

Thanks, I will ask the computing friends on the cmsweb schedules.

Our goal is to have stable software in Run 2, so that should
ease things in term of monitoring automatisation. The tau
object evolved a lot in the last couple of years so it was
difficult to obtain "useful" central-automatic validation/DQM.
But the plan is of course to go in that direction.

Cheers,
Monica

On 21 Jan 2014, at 17:07, deguio wrote:

ciao @monicavahttps://github.com/monicava
you are right. the updates to the DQM code follow the release cycle. layouts and render plugins follows the cmsweb updates instead and they don't depend on us actually. But in standard conditions I don't think that frequent updates are needed. This is the same for all the packages. Am I wrong?
anyway the pull is fine. I was wondering if moving to a GUI based validation is something planned.

thanks,
F.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1817#issuecomment-32899403.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_0_X IBs unless changes (tests are also fine). @ktf can you please take care of it?

@@ -33,7 +37,9 @@
{
// initialize the configurables
baseTauToken_ = consumes<edm::View<reco::BaseTau> >(iConfig.getParameter<edm::InputTag>( "tauSource" ));
pfTauToken_ = mayConsume<reco::PFTauCollection>(iConfig.getParameter<edm::InputTag>( "tauSource" ));
tauTransverseImpactParameterSrc_ = iConfig.getParameter<edm::InputTag>( "tauTransverseImpactParameterSource" );
tauTransverseImpactParameterToken_ = consumes<PFTauTIPAssociationByRef>( tauTransverseImpactParameterSrc_);
Copy link

Choose a reason for hiding this comment

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

Please use mayConsume, as #1988 shows that it can harm, if consumes is wrongly introduced.
This can be added as separate bug fix as soon as this one is merged, since it does not spoil tests here.

@davidlange6
Copy link
Contributor

+1

ktf added a commit that referenced this pull request Jan 27, 2014
Reco update -- High pt taus: TauID for 2014
@ktf ktf merged commit 477f5e7 into cms-sw:CMSSW_7_0_X Jan 27, 2014
jpavel added a commit to cms-tau-pog/cmssw that referenced this pull request Feb 2, 2014
jpavel added a commit to cms-tau-pog/cmssw that referenced this pull request Feb 3, 2014
@jpavel jpavel mentioned this pull request Feb 13, 2014
@jpavel jpavel mentioned this pull request Mar 28, 2014
@jpavel jpavel deleted the HighPtTaus_TauID2014 branch May 23, 2014 11:06
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.