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.

Sign up for GitHub

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

Jump to bottom

bug fix for Me0 segments #14306

Merged
merged 3 commits into from May 10, 2016
Merged

bug fix for Me0 segments #14306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d646c9e
bug fix for Me0 segments
jshlee Apr 29, 2016
cdfcf24
Merged ME0SegmentBugfix from repository jshlee
jshlee Apr 29, 2016
f677425
changed to lastrecostate.isValid
jshlee Apr 29, 2016
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
51 changes: 28 additions & 23 deletions RecoMuon/DetLayers/src/MuonME0DetLayerGeometryBuilder.cc
View file
Open in desktop
Expand Up @@ -32,22 +32,24 @@ MuonME0DetLayerGeometryBuilder::buildEndcapLayers(const ME0Geometry& geo) {
for (int endcap = -1; endcap<=1; endcap+=2) {
int iendcap = (endcap==1) ? 0 : 1; // +1: forward, -1: backward

vector<int> rolls;
std::vector<int> rings;
std::vector<int> chambers;
for(int roll = ME0DetId::minRollId+1; roll <= ME0DetId::maxRollId; ++roll) {
rolls.push_back(roll);
}
for(int chamber = ME0DetId::minChamberId+1; chamber <= ME0DetId::maxChamberId; chamber++ ){
chambers.push_back(chamber);
}
for(int layer = ME0DetId::minLayerId; layer <= ME0DetId::maxLayerId; ++layer) {
vector<int> rolls;
//std::vector<int> rings;
std::vector<int> chambers;
for(int roll = ME0DetId::minRollId+1; roll <= ME0DetId::maxRollId; ++roll) {
rolls.push_back(roll);
}
for(int chamber = ME0DetId::minChamberId+1; chamber <= ME0DetId::maxChamberId; chamber++ ){
chambers.push_back(chamber);
}

LogTrace(metname) << "Encap = " << endcap
<< "Chambers = " << chambers.size()
<< "Rolls = " << rolls.size();
MuRingForwardLayer* ringLayer = buildLayer(endcap, chambers, rolls, geo);
LogTrace(metname) << "Encap = " << endcap
<< "Chambers = " << chambers.size()
<< "Rolls = " << rolls.size();
MuRingForwardLayer* ringLayer = buildLayer(endcap, layer, chambers, rolls, geo);

if (ringLayer) result[iendcap].push_back(ringLayer);
if (ringLayer) result[iendcap].push_back(ringLayer);
}
}
pair<vector<DetLayer*>, vector<DetLayer*> > res_pair(result[0], result[1]);

Expand All @@ -57,6 +59,7 @@ MuonME0DetLayerGeometryBuilder::buildEndcapLayers(const ME0Geometry& geo) {

MuRingForwardLayer*
MuonME0DetLayerGeometryBuilder::buildLayer(int endcap,
int layer,
vector<int>& chambers,
vector<int>& rolls,
const ME0Geometry& geo) {
Expand All @@ -73,7 +76,7 @@ MuonME0DetLayerGeometryBuilder::buildLayer(int endcap,
vector<const GeomDet*> frontDets, backDets;

for(std::vector<int>::iterator chamber=chambers.begin(); chamber<chambers.end(); chamber++) {
ME0DetId me0Id(endcap,1,(*chamber), 0);
ME0DetId me0Id(endcap,layer,(*chamber), (*roll));
const GeomDet* geomDet = geo.idToDet(me0Id);

if (geomDet !=0) {
Expand Down Expand Up @@ -103,15 +106,17 @@ MuonME0DetLayerGeometryBuilder::buildLayer(int endcap,
}

}
LogTrace(metname) << "About to make a MuRingForwardLayer";
result = new MuRingForwardLayer(frontRings);

LogTrace(metname) << "New MuRingForwardLayer with " << frontRings.size()
<< " and " << backRings.size()
<< " rings, at Z " << result->position().z()
<< " R1: " << result->specificSurface().innerRadius()
<< " R2: " << result->specificSurface().outerRadius();

LogTrace(metname) << "About to make a MuRingForwardLayer";
if(frontRings.size()!=0) result = new MuRingForwardLayer(frontRings);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this MuRingForwardLayer object deleted? I couldn't find the deletion anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvuosalo - I only added the me0 layers.
I think it isnt deleted since this object is only created at the start.
This is also the case for all the other MuonDetLayerGeometryBuilders.

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears the design of the MuonDetLayerGeometryBuilders incorporates a memory leak. If so, the design should be fixed now before any more code is written based upon it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvuosalo - should I delete all these pointers at the destructor for all MuonDetLayerGeometryBuilders in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jshlee: Yes, it would be good to fix this issue. Changing to pointers to std::shared_ptr or std::unique_ptr (as appropriate) would mean the deletion would happen automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvuosalo - looking at this further, these are already assigned as std::shared_ptr upstream in https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/RecoMuon/DetLayers/plugins/MuonDetLayerGeometryESProducer.cc#L98

Copy link
Contributor

Choose a reason for hiding this comment

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

@jshlee: In answer to my original question, we found where the memory is deleted, so there is no problem:
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_0_pre4/RecoMuon/DetLayers/src/MuonDetLayerGeometry.cc#L33

else result = 0;
if(result != 0){
LogTrace(metname) << "New MuRingForwardLayer with " << frontRings.size()
<< " and " << backRings.size()
<< " rings, at Z " << result->position().z()
<< " R1: " << result->specificSurface().innerRadius()
<< " R2: " << result->specificSurface().outerRadius();
}
return result;

}
Expand Down
2 changes: 1 addition & 1 deletion RecoMuon/DetLayers/src/MuonME0DetLayerGeometryBuilder.h
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ class MuonME0DetLayerGeometryBuilder {
private:
//static MuRingForwardDoubleLayer* buildLayer(int endcap,int layer,std::vector<int>& chambers,std::vector<int>& rolls,const ME0Geometry& geo);
//static MuRingForwardLayer* buildLayer(int endcap,int layer,std::vector<int>& chambers,std::vector<int>& rolls,const ME0Geometry& geo);
static MuRingForwardLayer* buildLayer(int endcap,std::vector<int>& chambers,std::vector<int>& rolls,const ME0Geometry& geo);
static MuRingForwardLayer* buildLayer(int endcap, int layer, std::vector<int>& chambers,std::vector<int>& rolls,const ME0Geometry& geo);
static bool isFront(const ME0DetId & me0Id);
static MuDetRing * makeDetRing(std::vector<const GeomDet*> & geomDets);

Expand Down
2 changes: 2 additions & 0 deletions RecoMuon/MuonIdentification/plugins/ME0SegmentMatcher.cc
View file
Open in desktop
Expand Up @@ -164,6 +164,8 @@ void ME0SegmentMatcher::produce(edm::Event& ev, const edm::EventSetup& setup) {

//Remove later
if (std::abs(thisTrack->eta()) < 1.8) continue;
// low pt tracks cannot be propagated to me0
if (thisTrack->pt() < 1.0) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about using pt?
The p at eta~2.8 and pt=1 is about 8 GeV;
for the eta ~4 option, if it resurfaces, there is even larger ratio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Slava,

I didnt really check what the min was and assumed a lower pt wouldnt be used.
I think using pt would be the easiest and fastest way.
Do you have a better idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Jason,

|pz|>2.5 || p > 3 may be a more inclusive selection to expect the propagation to succeed.
You should anyways add a check lastrecostate.isValid after the propagation before using lastrecostate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lastrecostate.isValid works well. I changed to that.


float zSign = thisTrack->pz() > 0 ? 1.0f : -1.0f;

Expand Down