[RecoTauTag] allow complete relax of tau isolation at high pt #14529
Conversation
|
A new Pull Request was created by @rmanzoni (Riccardo Manzoni) for CMSSW_8_1_X. It involves the following packages: RecoTauTag/RecoTau @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
| @@ -39,6 +39,10 @@ class PFRecoTauDiscriminationByIsolation : public PFTauDiscriminationProducerBas | |||
| calculateWeights_ = pset.exists("ApplyDiscriminationByWeightedECALIsolation") ? | |||
| pset.getParameter<bool>("ApplyDiscriminationByWeightedECALIsolation") : false; | |||
|
|
|||
| // RIC: allow to relax the isolation completely beyond a given tau pt | |||
| minPtForNoIso_ = pset.exists("minTauPtForNoIso") ? | |||
There was a problem hiding this comment.
@roger-wolf @veelken @isobelojalvo @mbluj
remind me please of the status of ::fillDescriptions (the configuration validation system https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp )
for tau reco code.
If it's not on the tau POG list already, please add it.
Use of pset.exists is discouraged.
There was a problem hiding this comment.
Seconded. Please add fillDescription() method, and remove the exists(...) checks.
There was a problem hiding this comment.
@veelken @mbluj @isobelojalvo @slava77 @rmanzoni
Hi Slava,
we will open a ticket to fix both issues here and in other places in a general clean-up task within the next days. What are our chances to get this specific PR integrated with this caveat? I just learned from Riccardo that it will be difficult for him to do it before for this specific case due to limited internet access this week. On the other hand if I understand him correctly he needs to meet a deadline with this PR basically tomorrow.
There was a problem hiding this comment.
I think that it should be OK to leave the fillDescription migration for the next PR.
Tau code has O(10) producers inheriting from PFTauDiscriminationProducerBase.
A full PSet will need to be defined with that fillDescription, both in the base class and in this (and likely other as well) derived classes.
So, it's not a matter of a small set of lines of code change.
Don't expect to easily include more parameters like this in the future.
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Also need a 80X PR. |
|
To make the change visible by ConfDB the newly added argument should appear either in the cfi python file of the module or be filled by the fillDescription() method which is not implemented for the module. Although the latter is preferred, it is beyond of this relatively small PR and we (Tau) propose to handle it separately. |
|
Pull request #14529 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
| @@ -26,6 +26,8 @@ | |||
| relativeSumPtCut = cms.double(0.0), | |||
| relativeSumPtOffset = cms.double(0.0), | |||
|
|
|||
| minTauPtForNoIso = cms.bool(-99.), # minimum tau pt at which the isolation is completely relaxed. If negative, this is disabled | |||
There was a problem hiding this comment.
this is clearly wrong, as a boolean cannot have the value -99.
There was a problem hiding this comment.
please test the changes before committing them.
|
Pull request #14529 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar |
cmsbuild
added
reconstruction-approved
fully-signed
and removed
pending-signatures
reconstruction-pending
labels
|
+1 |
Add configurable option to completely relax the isolation when tau pt is above a given threshold, i.e. the discriminator always returns true.
The configuration parameter is optional and therefore backwards compatible.