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

Some EcalClusterTools maintainance and energy matrix in Egamma MVA Ntuplizers #25633

Merged
merged 3 commits into from Mar 3, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
112 changes: 112 additions & 0 deletions RecoCaloTools/Navigation/interface/CaloRectangle.h
@@ -0,0 +1,112 @@
#ifndef RecoCaloTools_Navigation_CaloRectangle_H
#define RecoCaloTools_Navigation_CaloRectangle_H

#include "Geometry/CaloTopology/interface/CaloSubdetectorTopology.h"
#include "Geometry/CaloTopology/interface/CaloTopology.h"

/*
* CaloRectangle is a class to create a rectangular range of DetIds around a
* central DetId. Meant to be used for range-based loops to calculate cluster
* shape variables.
*/

struct CaloRectangle {
const int iEtaOrIXMin;
const int iEtaOrIXMax;
const int iPhiOrIYMin;
const int iPhiOrIYMax;

template<class T>
auto operator()(T home, CaloTopology const& topology);

};

template<class T>
T offsetBy(T start, CaloSubdetectorTopology const& topo, int dIEtaOrIX, int dIPhiOrIY)
{
for(int i = 0; i < std::abs(dIEtaOrIX) && start != T(0); i++) {
start = dIEtaOrIX > 0 ? topo.goEast(start) : topo.goWest(start);
}

for(int i = 0; i < std::abs(dIPhiOrIY) && start != T(0); i++) {
start = dIPhiOrIY > 0 ? topo.goNorth(start) : topo.goSouth(start);
}
return start;
}

template<class T>
class CaloRectangleRange {

public:

class Iterator {

public:

Iterator(T const& home, int iEtaOrIX, int iPhiOrIY, CaloRectangle const rectangle, CaloSubdetectorTopology const& topology)
: home_(home)
, rectangle_(rectangle)
, topology_(topology)
, iEtaOrIX_(iEtaOrIX)
, iPhiOrIY_(iPhiOrIY)
{}

Iterator& operator++() {
if(iPhiOrIY_ == rectangle_.iPhiOrIYMax) {
iPhiOrIY_ = rectangle_.iPhiOrIYMin;
iEtaOrIX_++;
} else ++iPhiOrIY_;
return *this;
}

int iEtaOrIX() const { return iEtaOrIX_; }
int iPhiOrIY() const { return iPhiOrIY_; }

Copy link
Contributor

Choose a reason for hiding this comment

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

So a usability comment. This is ieta, iphi in the barrel, not ix, iy which is just the endcap.

This might cause user confusion. Obviously there is no good solution here as the variable name changes based on the subdector. In my private code, I've always used iEtaOrIX() / iPhiOrIY() to solve this issue. However I'm more than happy to hear other peoples opinions / suggestions on what is the best course of action here for the names.

bool operator==(Iterator const& other) const { return iEtaOrIX_ == other.iEtaOrIX() && iPhiOrIY_ == other.iPhiOrIY(); }
bool operator!=(Iterator const& other) const { return iEtaOrIX_ != other.iEtaOrIX() || iPhiOrIY_ != other.iPhiOrIY(); }

T operator*() const { return offsetBy(home_, topology_, iEtaOrIX_, iPhiOrIY_); }

private:

const T home_;

const CaloRectangle rectangle_;
CaloSubdetectorTopology const& topology_;

int iEtaOrIX_;
int iPhiOrIY_;
};

public:
CaloRectangleRange(CaloRectangle rectangle, T home, CaloTopology const& topology)
: home_(home)
, rectangle_(rectangle)
, topology_(*topology.getSubdetectorTopology(home))
{}

CaloRectangleRange(int size, T home, CaloTopology const& topology)
: home_(home)
, rectangle_{-size, size, -size, size}
, topology_(*topology.getSubdetectorTopology(home))
{}

auto begin() {
return Iterator(home_, rectangle_.iEtaOrIXMin, rectangle_.iPhiOrIYMin, rectangle_, topology_);
}
auto end() {
return Iterator(home_, rectangle_.iEtaOrIXMax + 1, rectangle_.iPhiOrIYMin, rectangle_, topology_);
}

private:
const T home_;
const CaloRectangle rectangle_;
CaloSubdetectorTopology const& topology_;
Copy link
Contributor

Choose a reason for hiding this comment

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

One of my long standing issues with EcalClusterTools is that it fairly needlessly uses CaloSubdetectorTopology and we could just hardcode it. I mean iEta 83 will always be next to iEta 84.

This could have complications in the phase II for the endcap though we would have to look into this. Anyways one possibility is that we could use this class to hide the CaloTopology from EcalClusterTools (would obv require a bit of a re-write) so it could either pick up a CaloTopology object or a hard coded class. Although maybe better ways to achieve this.

Anyways something to think about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea for the future! Yes, that's probably one of the nice things about this class, that these kind of considerations can be brought in here without cluttering the ClusterTools.

And what about iEta 0 by the way? That does not exist right? what happens then, will it be jumped over?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you have limits on it of course. My version (which I use in my private code) knows to skip iEta=0, wrap phi in 1-360 and that 85 is the last crystal. I have versions for HCAL, L1, etc as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

eg
https://github.com/Sam-Harper/usercode/blob/100XNtup/SHNtupliser/interface/DetIdTools.hh#L15
https://github.com/Sam-Harper/usercode/blob/100XNtup/SHNtupliser/src/DetIdTools.cc#L72

Incidentally thats the code what was used to develop several of the showershapes so its the "correct version" if there are bugs :)

};

template<class T>
auto CaloRectangle::operator()(T home, CaloTopology const& topology) {
return CaloRectangleRange<T>(*this, home, topology);
}

#endif
21 changes: 6 additions & 15 deletions RecoEcal/EgammaClusterProducers/src/EcalDigiSelector.cc
Expand Up @@ -134,14 +134,9 @@ void EcalDigiSelector::produce(edm::Event& evt, const edm::EventSetup& es)
const CaloClusterPtr& bcref = clus1.seed();
const BasicCluster *bc = bcref.get();
//Get the maximum detid
std::pair<DetId, float> EDetty =
EcalClusterTools::getMaximum(*bc,rechits);
//get the 3x3 array centered on maximum detid.
std::vector<DetId> detvec =
EcalClusterTools::matrixDetId(topology,EDetty.first, -1, 1, -1, 1);
//Loop over the 3x3
for (int ik = 0;ik<int(detvec.size());++ik)
saveTheseDetIds.push_back(detvec[ik]);
DetId maxDetId = EcalClusterTools::getMaximum(*bc,rechits).first;
// Loop over the 3x3 array centered on maximum detid
for(DetId detId : CaloRectangleRange(1, maxDetId, *topology)) saveTheseDetIds.push_back(detId);

}
for (int detloop=0; detloop < int(saveTheseDetIds.size());++detloop){
Expand Down Expand Up @@ -196,13 +191,9 @@ void EcalDigiSelector::produce(edm::Event& evt, const edm::EventSetup& es)
const CaloClusterPtr& bcref = clus1.seed();
const BasicCluster *bc = bcref.get();
//Get the maximum detid
std::pair<DetId, float> EDetty = EcalClusterTools::getMaximum(*bc,rechits);
//get the 3x3 array centered on maximum detid.
std::vector<DetId> detvec =
EcalClusterTools::matrixDetId(topology,EDetty.first, -1, 1, -1, 1);
//Loop over the 3x3
for (int ik = 0;ik<int(detvec.size());++ik)
saveTheseDetIds.insert(detvec[ik]);
DetId maxDetId = EcalClusterTools::getMaximum(*bc,rechits).first;
// Loop over the 3x3 array centered on maximum detid
for(DetId detId : CaloRectangleRange(1, maxDetId, *topology)) saveTheseDetIds.insert(detId);
}
int eecounter=0;
for (EEDigiCollection::const_iterator blah = digis->begin();
Expand Down