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 photons to tau jet cones #128

Merged
merged 8 commits into from Nov 7, 2022
Merged

Conversation

msessini
Copy link
Contributor

No description provided.

Comment on lines 100 to 130
template<typename Gamma, int cmssw_version>
struct GetGammaVer;

//CMSSW 10 --> 12
//hcalOverEcal() --> hadronicOverEm()
//hcalOverEcalBc() --> hadTowOverEm()
template<typename Gamma>
struct GetGammaVer<Gamma, 12> {
static float hcalDepth1OverEcal(const Gamma* photon) { return photon->hadronicOverEm(1); }
static float hcalDepth2OverEcal(const Gamma* photon) { return photon->hadronicOverEm(2); }
static float hcalDepth1OverEcalBc(const Gamma* photon) { return photon->hadTowOverEm(1); }
static float hcalDepth2OverEcalBc(const Gamma* photon) { return photon->hadTowOverEm(2); }
static float full5x5_hcalDepth1OverEcal(const Gamma* photon) { return photon->full5x5_hadronicOverEm(1); }
static float full5x5_hcalDepth2OverEcal(const Gamma* photon) { return photon->full5x5_hadronicOverEm(2); }
static float full5x5_hcalDepth1OverEcalBc(const Gamma* photon) { return photon->full5x5_hadTowOverEm(1); }
static float full5x5_hcalDepth2OverEcalBc(const Gamma* photon) { return photon->full5x5_hadTowOverEm(2); }
};

template<typename Gamma>
struct GetGammaVer<Gamma, 10> {
static float hcalDepth1OverEcal(const Gamma* photon) { return photon->hadronicDepth1OverEm(); }
static float hcalDepth2OverEcal(const Gamma* photon) { return photon->hadronicDepth2OverEm(); }
static float hcalDepth1OverEcalBc(const Gamma* photon) { return photon->hadTowDepth1OverEm(); }
static float hcalDepth2OverEcalBc(const Gamma* photon) { return photon->hadTowDepth2OverEm(); }
static float full5x5_hcalDepth1OverEcal(const Gamma* photon) { return -999.; }
static float full5x5_hcalDepth2OverEcal(const Gamma* photon) { return -999.; }
static float full5x5_hcalDepth1OverEcalBc(const Gamma* photon) { return -999.; }
static float full5x5_hcalDepth2OverEcalBc(const Gamma* photon) { return -999.; }
};


Copy link
Contributor

Choose a reason for hiding this comment

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

@msessini please resolve the existing conflict in this part of the code.

And add overloading for version == 11 like we do for Electrons:

    template<typename Elec>
    struct GetElecVer<Elec, 11> : GetElecVer<Elec, 10> {};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I forgot to raise one point: do we want to keep compatibility with releases before CMSSW_12_4? UL MiniAODs can be processed with 12_4, so should be safe from that point. I think we can drop compatibility with previous releases, including python 2 (finally!)

Copy link
Contributor

Choose a reason for hiding this comment

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

It would make sense in general, but I still use UL for displaced taus. Also: what is the release for phase 2, isn't it 11__?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shedprog - for UL it is safe to move to 12_4_X for miniAOD->TauTuple step. Actually we need to do this to include DeepTau v2p5 in the next ntuples.
@DennRoy - is it ok to move to >= 12_X for the future Phase 2 studies?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are at least two changes required to make Phase2 production work with CMSSW 12. These are discussed here:
https://cms-talk.web.cern.ch/t/getting-muon-station-value-0/15765/1

Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you @DennRoy ! I think this can be implemented in this PR and then we can drop compatibility for CMSSW < 12, if there are no other objections.

Copy link
Contributor Author

@msessini msessini Oct 4, 2022

Choose a reason for hiding this comment

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

@DennRoy, which CMSSW_12_4_X release would be best for phase2 production as replacement of CMSSW_11_2_5 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about any "best" release. I've used 12_4_0 and 12_4_8 before, which seems to have worked equally well. But I've also been told that 12_6_0_pre3 MIGHT fix the low tau reco efficiency in the Phase2 endcap (to be confirmed).

Analysis/interface/TauTuple.h Show resolved Hide resolved
Analysis/interface/TauTuple.h Show resolved Hide resolved
Production/interface/Selectors.h Outdated Show resolved Hide resolved
Production/plugins/TauTupleProducer.cc Outdated Show resolved Hide resolved
Production/plugins/TauTupleProducer.cc Outdated Show resolved Hide resolved
env.sh Outdated Show resolved Hide resolved
Production/src/TauJet.cc Outdated Show resolved Hide resolved
Production/src/TauJet.cc Outdated Show resolved Hide resolved
@msessini
Copy link
Contributor Author

General question, it seems that EDAnalyzer is now deprecated (??), do you know what should be used instead ?

warning: 'edm::EDAnalyzer::EDAnalyzer()' is deprecated [-Wdeprecated-declarations]

@ofivite
Copy link
Member

ofivite commented Oct 14, 2022

General question, it seems that EDAnalyzer is now deprecated (??), do you know what should be used instead ?

Hmm this is something unusual.. I personally haven't heard about that, so searching for this warning led me to some PRs in CMSSW doing an API change to edm::one::EDAnalyzer, for example: cms-sw/cmssw#35343 , cms-sw/cmssw#37044

And then I found this twiki page: https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkOneModuleInterface

It looks like they talk about thread safety, but I would need to read this further. @kandrosov do you think it affects us and we would need to migrate too?

env.sh Outdated Show resolved Hide resolved
Comment on lines 1057 to 1076
for(const auto& sv_ptr : sv_col) {
const reco::VertexCompositePtrCandidate* SV = sv_ptr.obj;
auto candsIdx = collectCands(*SV);
if(candsIdx.size() > 0){
tauTuple().sv_x.push_back(static_cast<float>(SV->position().x()));
tauTuple().sv_y.push_back(static_cast<float>(SV->position().y()));
tauTuple().sv_z.push_back(static_cast<float>(SV->position().z()));
tauTuple().sv_t.push_back(static_cast<float>(SV->t()));
tauTuple().sv_xE.push_back(static_cast<float>(std::sqrt(SV->vertexCovariance(0,0))));
tauTuple().sv_yE.push_back(static_cast<float>(std::sqrt(SV->vertexCovariance(1,1))));
tauTuple().sv_zE.push_back(static_cast<float>(std::sqrt(SV->vertexCovariance(2,2))));
tauTuple().sv_tE.push_back(static_cast<float>(SV->tError()));
tauTuple().sv_chi2.push_back(static_cast<float>(SV->vertexChi2()));
tauTuple().sv_ndof.push_back(static_cast<float>(SV->vertexNdof()));
for(int cand_idx : candsIdx) {
tauTuple().sv_cands_svIdx.push_back(sv_ptr.index);
tauTuple().sv_cands_pfcandIdx.push_back(cand_idx);
}
++nsv;
}
Copy link
Contributor

@shedprog shedprog Oct 17, 2022

Choose a reason for hiding this comment

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

@ofivite correct me if I am wrong, in this way -> SVs that matched to PFCand that selected within cone will be filled only, so then the difference I was thinking to also add the SVs that are within the cone but do not have any pfcands. Also, this structure with indexes is a bit weird, I would have indexes for Pfcands pointing SV then it is just

Copy link
Collaborator

Choose a reason for hiding this comment

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

we have chosen SV<->pfCand mapping as a separate vectors, because we were not sure if pfCand->SV is guaranteed to be unique. Do you know if it is unique by design?

@kandrosov
Copy link
Collaborator

It looks like they talk about thread safety, but I would need to read this further. @kandrosov do you think it affects us and we would need to migrate too?

it is usually safer to run crab tasks with one thread to reduce memory limit exceptions. I would say it is not urgent to migrate, but we can add it to our todo list to stay aligned with the mainstream CMSSW code.

@@ -47,7 +47,7 @@ void MuonHitMatch::CountMatches(const pat::Muon& muon, CountMap& n_matches)
{
for(const auto& segment : muon.matches()) {
if(segment.segmentMatches.empty() && segment.rpcMatches.empty() && segment.gemMatches.empty() && segment.me0Matches.empty()) continue;
if(n_matches.count(segment.detector())) {
if(n_matches.count(segment.detector()) && segment.station()!=0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we understand why it is needed? If 0 is a valid station, why it should not be saved?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if "0" is really valid (My understanding is insufficient to answer this). In order to fix the crash that happens otherwise here, the alternative would be to allow the station 0 here.

Copy link
Collaborator

@kandrosov kandrosov Oct 17, 2022

Choose a reason for hiding this comment

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

I agree that the crush must be fixed. But it would be preferable to make a conscious choice whatever we want to ignore stations "0" or they could be useful, and then modify the code accordingly.

Production/src/TauJet.cc Outdated Show resolved Hide resolved
env.sh Outdated Show resolved Hide resolved
Production/src/TauJet.cc Outdated Show resolved Hide resolved
Comment on lines +1061 to +1064
tauTuple().sv_x.push_back(static_cast<float>(SV->position().x()));
tauTuple().sv_y.push_back(static_cast<float>(SV->position().y()));
tauTuple().sv_z.push_back(static_cast<float>(SV->position().z()));
tauTuple().sv_t.push_back(static_cast<float>(SV->t()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also store pt, eta, phi, mass

Production/plugins/TauTupleProducer.cc Outdated Show resolved Hide resolved
Production/plugins/TauTupleProducer.cc Outdated Show resolved Hide resolved
Production/src/TauJet.cc Outdated Show resolved Hide resolved
Production/plugins/TauTupleProducer.cc Outdated Show resolved Hide resolved
Production/plugins/TauTupleProducer.cc Outdated Show resolved Hide resolved
env.sh Outdated Show resolved Hide resolved
@shedprog shedprog merged commit db4ed90 into cms-tau-pog:master Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants