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

New E/g HLT Pixel Matching : 92X #18541

Merged
merged 19 commits into from May 5, 2017
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
202 changes: 130 additions & 72 deletions DataFormats/EgammaReco/interface/ElectronSeed.h
@@ -1,14 +1,38 @@
#ifndef ElectronSeed_h
#define ElectronSeed_h 1

/** \class reco::ElectronSeed
*
* ElectronSeed is a seed for gsf tracking, constructed from
* either a supercluster or a ctf track.
*
* \author D.Chamont, U.Berthon, C.Charlot, LLR Palaiseau
*
************************************************************/
#ifndef DataFormats_EgammaReco_ElectronSeed_h
#define DataFormats_EgammaReco_ElectronSeed_h

//********************************************************************
//
// A verson of reco::ElectronSeed which can have N hits as part of the
// 2017 upgrade of E/gamma pixel matching for the phaseI pixels
//
// author: S. Harper (RAL), 2017
//
//notes:
// While it is technically named ElectronSeed, it is effectively a new class
// However to simplify things, the name ElectronSeed was kept
// (trust me it was simplier...)
//
// Noticed that h/e values never seem to used anywhere and they are a
// mild pain to propagate in the new framework so they were removed
//
// infinities are used to mark invalid unset values to maintain
// compatibilty with the orginal ElectronSeed class
//
//description:
// An ElectronSeed is a TrajectorySeed with E/gamma specific information
// A TrajectorySeed has a series of hits associated with it
// (accessed by TrajectorySeed::nHits(), TrajectorySeed::recHits())
// and ElectronSeed stores which of those hits match well to a supercluster
// together with the matching parameters (this is known as EcalDriven).
// ElectronSeeds can be TrackerDriven in which case the matching is not done.
// It used to be fixed to two matched hits, now this is an arbitary number
// Its designed with pixel matching with mind but tries to be generally
// applicable to strips as well.
// It is worth noting that due to different ways ElectronSeeds can be created
// they do not always have all parameters filled
//
//********************************************************************

#include "DataFormats/EgammaReco/interface/ElectronSeedFwd.h"
#include "DataFormats/CaloRecHit/interface/CaloClusterFwd.h"
Expand All @@ -23,93 +47,127 @@
#include <limits>

namespace reco
{

class ElectronSeed : public TrajectorySeed
{
{

class ElectronSeed : public TrajectorySeed {
public :

struct PMVars {
float dRZPos;
float dRZNeg;
float dPhiPos;
float dPhiNeg;
int detId; //this is already stored as the hit is stored in traj seed but a useful sanity check
int layerOrDiskNr;//redundant as stored in detId but its a huge pain to hence why its saved here

PMVars();
void setDPhi(float pos,float neg){dPhiPos=pos;dPhiNeg=neg;}
void setDRZ(float pos,float neg){dRZPos=pos;dRZNeg=neg;}
void setDet(int iDetId,int iLayerOrDiskNr){detId=iDetId;layerOrDiskNr=iLayerOrDiskNr;}
Copy link
Contributor

Choose a reason for hiding this comment

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

some spaces here and below would be nice to improve readability


};


typedef edm::OwnVector<TrackingRecHit> RecHitContainer ;
typedef edm::RefToBase<CaloCluster> CaloClusterRef ;
typedef edm::Ref<TrackCollection> CtfTrackRef ;

static std::string const & name()
{
{
static std::string const name_("ElectronSeed") ;
return name_;
}

}
//! Construction of base attributes
ElectronSeed() ;
ElectronSeed( const TrajectorySeed & ) ;
ElectronSeed( PTrajectoryStateOnDet & pts, RecHitContainer & rh, PropagationDirection & dir ) ;
ElectronSeed * clone() const { return new ElectronSeed(*this) ; }
virtual ~ElectronSeed() ;
virtual ~ElectronSeed()=default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave

virtual ~ElectronSeed();

in the .h file, and write

ElectronSeed::~ElectronSeed() = default;

in the .cc file ?

The reason is that gcc emits (or used to) the vtable in the compilation unit where it sees the definition of the first virtual function.
If the only virtual function is the destructor, and it is fully defined in the header file, I have no idea what happens... possibly the vtable is emitted in all compilation units that #include the header file, and then the linker is left to clean up the duplications ?


//! Set additional info
void setCtfTrack( const CtfTrackRef & ) ;
void setCaloCluster
( const CaloClusterRef &,
unsigned char hitsMask =0,
int subDet2 =0, int subDet1 =0,
float hoe1 =std::numeric_limits<float>::infinity(),
float hoe2 =std::numeric_limits<float>::infinity() ) ;
void setNegAttributes
( float dRz2 =std::numeric_limits<float>::infinity(),
float dPhi2 =std::numeric_limits<float>::infinity(),
float dRz1 =std::numeric_limits<float>::infinity(),
float dPhi1 =std::numeric_limits<float>::infinity() ) ;
void setPosAttributes
( float dRz2 =std::numeric_limits<float>::infinity(),
float dPhi2 =std::numeric_limits<float>::infinity(),
float dRz1 =std::numeric_limits<float>::infinity(),
float dPhi1 =std::numeric_limits<float>::infinity() ) ;

void setCaloCluster( const CaloClusterRef& clus){caloCluster_=clus;isEcalDriven_=true;}
void addHitInfo(const PMVars& hitVars){hitInfo_.push_back(hitVars);}
void setNrLayersAlongTraj(int val){nrLayersAlongTraj_=val;}
//! Accessors
const CtfTrackRef& ctfTrack() const { return ctfTrack_ ; }
const CaloClusterRef& caloCluster() const { return caloCluster_ ; }
unsigned char hitsMask() const { return hitsMask_ ; }
int subDet2() const { return subDet2_ ; }
float dRz2() const { return dRz2_ ; }
float dPhi2() const { return dPhi2_ ; }
float dRz2Pos() const { return dRz2Pos_ ; }
float dPhi2Pos() const { return dPhi2Pos_ ; }
int subDet1() const { return subDet1_ ; }
float dRz1() const { return dRz1_ ; }
float dPhi1() const { return dPhi1_ ; }
float dRz1Pos() const { return dRz1Pos_ ; }
float dPhi1Pos() const { return dPhi1Pos_ ; }
float hoe1() const { return hcalDepth1OverEcal_ ; }
float hoe2() const { return hcalDepth2OverEcal_ ; }


//! Utility
TrackCharge getCharge() const { return startingState().parameters().charge() ; }

bool isEcalDriven() const { return isEcalDriven_ ; }
bool isTrackerDriven() const { return isTrackerDriven_ ; }

private:
const std::vector<PMVars>& hitInfo()const{return hitInfo_;}
float dPhiNeg(size_t hitNr)const{return getVal(hitNr,&PMVars::dPhiNeg);}
float dPhiPos(size_t hitNr)const{return getVal(hitNr,&PMVars::dPhiPos);}
float dPhiBest(size_t hitNr)const{return bestVal(dPhiNeg(hitNr),dPhiPos(hitNr));}
float dRZPos(size_t hitNr)const{return getVal(hitNr,&PMVars::dRZPos);}
float dRZNeg(size_t hitNr)const{return getVal(hitNr,&PMVars::dRZNeg);}
float dRZBest(size_t hitNr)const{return bestVal(dRZNeg(hitNr),dRZPos(hitNr));}
int detId(size_t hitNr)const{return hitNr<hitInfo_.size() ? hitInfo_[hitNr].detId : 0;}
int subDet(size_t hitNr)const{return DetId(detId(hitNr)).subdetId();}
int layerOrDiskNr(size_t hitNr)const{return getVal(hitNr,&PMVars::layerOrDiskNr);}
int nrLayersAlongTraj()const{return nrLayersAlongTraj_;}

//redundant, backwards compatible function names
//to be cleaned up asap
//no new code should use them
//they were created as time is short and there is less risk having
//the functions here rather than adapting all the function call to them in other
//CMSSW code
float dPhi1()const{return dPhiNeg(0);}
float dPhi1Pos()const{return dPhiPos(0);}
float dPhi2()const{return dPhiNeg(1);}
float dPhi2Pos()const{return dPhiPos(1);}
float dRz1()const{return dRZNeg(0);}
float dRz1Pos()const{return dRZPos(0);}
float dRz2()const{return dRZNeg(1);}
float dRz2Pos()const{return dRZPos(1);}
int subDet1()const{return subDet(0);}
int subDet2()const{return subDet(1);}
int hitsMask()const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why signed? (Perhaps: "unsigned int", if you need more bits than previous "unsigned char")?
See also L97 of DataFormats/EgammaReco/src/classes_def.xml

Copy link
Contributor Author

@Sam-Harper Sam-Harper May 3, 2017

Choose a reason for hiding this comment

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

Simply a typo on my part. Thanks for the spot! I'll fix this shortly.

Copy link
Contributor Author

@Sam-Harper Sam-Harper May 3, 2017

Choose a reason for hiding this comment

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

So I looked into this further. I think I would like to have it as a "unsigned int" as there can not be >32 layers in a seed while >8 is possible and therefore needs to be projected against?

I looked where it is used: http://cmslxr.fnal.gov/search?_filestring=&_string=hitsMask%28%29 and its really not so I think its safe to change to an unsigned int. Thoughts?

Also I realized this function: void initTwoHitSeed(const char hitMask); needs to an unsigned char, it wouldnt work if we ever had a 8 hit seed. Will wait an hour or two and then make the change

Copy link
Contributor

Choose a reason for hiding this comment

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

About void initTwoHitSeed(const char hitMask) , I also noticed it.

However, if I understand correctly, there the argument of the method is
https://github.com/Sam-Harper/cmssw/blob/44f047ea18445000e597c85e513f7711580cb8e8/RecoEgamma/EgammaElectronAlgos/src/ElectronSeedGenerator.cc#L479

i.e. obtained from a SeedWithInfo object:
http://cmsdoxygen.web.cern.ch/cmsdoxygen/CMSSW_9_1_0_pre3/doc/html/d4/ddb/classSeedWithInfo.html

Unless you also plan to modify that class, it should stay as such: correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so looking into it. It doesnt make any difference if its a char or unsigned char there as we are dealing with the bit representation. I was worried about the later conversion to a size_t. However I would like to have it const unsigned char just to be consistent. Also I found that function is a bit buggy and my testing didnt reveal it so will fix it

void initTwoHitSeed(const char hitMask);
void setNegAttributes(const float dRZ2=std::numeric_limits<float>::infinity(),
const float dPhi2=std::numeric_limits<float>::infinity(),
const float dRZ1=std::numeric_limits<float>::infinity(),
const float dPhi1=std::numeric_limits<float>::infinity());
void setPosAttributes(const float dRZ2=std::numeric_limits<float>::infinity(),
const float dPhi2=std::numeric_limits<float>::infinity(),
const float dRZ1=std::numeric_limits<float>::infinity(),
const float dPhi1=std::numeric_limits<float>::infinity());



CtfTrackRef ctfTrack_ ;
CaloClusterRef caloCluster_ ;
unsigned char hitsMask_ ;
int subDet2_ ;
float dRz2_ ;
float dPhi2_ ;
float dRz2Pos_ ;
float dPhi2Pos_ ;
int subDet1_ ;
float dRz1_ ;
float dPhi1_ ;
float dRz1Pos_ ;
float dPhi1Pos_ ;
float hcalDepth1OverEcal_ ; // hcal over ecal seed cluster energy using first hcal depth
float hcalDepth2OverEcal_ ; // hcal over ecal seed cluster energy using 2nd hcal depth
bool isEcalDriven_ ;
bool isTrackerDriven_ ;
private:
static float bestVal(float val1,float val2){return std::abs(val1)<std::abs(val2) ? val1 : val2;}
template<typename T>
T getVal(size_t hitNr,T PMVars::*val)const{
return hitNr<hitInfo_.size() ? hitInfo_[hitNr].*val : std::numeric_limits<T>::infinity();
}
static std::vector<size_t> hitNrsFromMask(size_t hitMask);
Copy link
Contributor

Choose a reason for hiding this comment

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

size_t is platform-dependent.
Better use explicit size type for data format

public:
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not interleave public-private-public sections

//this is a backwards compatible function designed to
//convert old format ElectronSeeds to the new format
//only public due to root io rules, not intended for any other use
//also in theory not necessary to part of this class
static std::vector<PMVars> createHitInfo(const float dPhi1Pos,const float dPhi1Neg,
const float dRZ1Pos,const float dRZ1Neg,
const float dPhi2Pos,const float dPhi2Neg,
const float dRZ2Pos,const float dRZ2Neg,
const char hitMask,const TrajectorySeed::range recHits);

private:

} ;
CtfTrackRef ctfTrack_;
CaloClusterRef caloCluster_;
std::vector<PMVars> hitInfo_;
int nrLayersAlongTraj_;

bool isEcalDriven_;
bool isTrackerDriven_;

}
};
}

#endif
2 changes: 1 addition & 1 deletion DataFormats/EgammaReco/interface/ElectronSeedFwd.h
Expand Up @@ -17,7 +17,7 @@ namespace reco {
/// vector of objects in the same collection of ElectronSeed objects
typedef edm::RefVector<ElectronSeedCollection> ElectronSeedRefVector;
/// iterator over a vector of reference to ElectronSeed objects
typedef ElectronSeedRefVector::iterator electronephltseed_iterator;
typedef ElectronSeedRefVector::iterator electronseed_iterator;
}

#endif