GSI Forum
GSI Helmholtzzentrum für Schwerionenforschung

Home » PANDA » PandaRoot » Bugs, Fixes, Releases » Bug in PndEmcHitProducer
Bug in PndEmcHitProducer [message #10529] Wed, 14 April 2010 11:00 Go to next message
Vanniarajan Suyam Jothi is currently offline  Vanniarajan Suyam Jothi
Messages: 35
Registered: October 2007
Location: Groningen
continuous participant

From: *
There was a probable problem in the PndEmcHitProducer observed.
The Energy deposited in the crystal from the PndEmcHit is fractionaly more than the energy sum from the PndEmcPoint
in the crystal.
Mohammad Babai has found that there is a problem in the
following part of the PndEmcHitProducer::Exec() method.
the data entries of two maps fTrackEnergy and fTrackTime
are accessed un-initialised. Now He has fixed the

for (Int_t iPoint=0; iPoint<nPoints; iPoint++)
point = (PndEmcPoint*) fPointArray->At(iPoint);
fTrackEnergy[point->GetDetectorID()] += point->GetEnergyLoss();
point_time=point ->GetTime();
if (point_time<fTrackTime[point->GetDetectorID()]) fTrackTime[point->GetDetectorID()] =point_time;

// Check and save MC truth information
// Eloss==0 tracks are only stored in point, if track is entering detector from outside
// and thats what we are interested in...
// cout << "ELoss==0 : ID " << point->GetTrackID()<<","<<point->GetDetectorID()<<","<<point->GetXPad() <<","<<point->GetYPad()<<endl;


[Updated on: Wed, 14 April 2010 11:27]

Report message to a moderator

Re: Bug in PndEmcHitProducer [message #10530 is a reply to message #10529] Wed, 14 April 2010 11:40 Go to previous messageGo to next message
M.Babai is currently offline  M.Babai
Messages: 46
Registered: January 2008
Location: Netherlands
continuous participant
From: *

This problem is now solved and the corrected code is available in trunk.
In the original code the following was declared and used without proper initialization:

map<Int_t, Float_t> fTrackEnergy;
map<Int_t, Float_t> fTrackTime;  //time of first point
map<Int_t, std::vector <Int_t> > fTrackMcTruth;  //McTruth
point  = (PndEmcPoint*) fPointArray->At(iPoint);
fTrackEnergy[point->GetDetectorID()] += point->GetEnergyLoss();
point_time=point ->GetTime();
if (point_time < fTrackTime[point->GetDetectorID()])
    fTrackTime[point->GetDetectorID()] = point_time;

In the lines above we can see a comparison and a "+=" operation on not initialized member of the map which has(might have) an undefined state.
Another point, which is (in this case) a matter of taste and beauty, is the declaration of:
PndEmcPoint* point = NULL;
map<Int_t, Float_t>::const_iterator p;
outside the loop. In this case they are not leading into wrong computations but potentially they can, as their values are changed inside the loop.


[Updated on: Wed, 14 April 2010 11:43]

Report message to a moderator

Re: Bug in PndEmcHitProducer [message #10533 is a reply to message #10529] Wed, 14 April 2010 20:01 Go to previous messageGo to next message
Elwin Dijck
Messages: 16
Registered: June 2009
Location: Groningen, The Netherland...
occasional visitor
From: *
Hi all,

I don't think there was any problem with undefined values: if you try to access an element in a std::map that is not in the map using the []-operator, that element will be added with its default value. The default 'constructor' float() is not undefined and will simply give 0.0, so the usage of fTrackEnergy with += will work properly without explicit initialization.

However it is indeed true that fTrackTime needs initialization here, in the old code the result would always be the default value of zero since point_time is never < 0.

Re: Bug in PndEmcHitProducer [message #10534 is a reply to message #10533] Wed, 14 April 2010 22:05 Go to previous message
Johan Messchendorp is currently offline  Johan Messchendorp
Messages: 693
Registered: April 2007
Location: University of Groningen
first-grade participant

From: *

I tend to agree with Elwin. I did some tests with a simple program using a map simulating the code and indeed a first time call of the []-operator gives on all tested platforms nicely gives a zero. So, I also think that this cannot be a problem. The time issue is more nasty, but then again, if you don't use the time information in the analysis, it should not be a problem. So, honestly speaking, I cannot understand why the energy deposited using the PndEmcHitProducer should differ from looping over the points manually. The problem must be hidden somewhere else.

Mohammad A. pointed out to me another potential problem in the PndEmcHitProducer code related to the filling of the container of the hits:


// ------------------------------------------------------------------------ -
// ----- Private method AddDigi --------------------------------------------
PndEmcHit* PndEmcHitProducer::AddHit(Int_t trackID,Int_t detID, Float_t energy,
Float_t time, std::vector <Int_t> &mctruth)
// It fills the PndEmcHit category

//cout << "PndEmcHitProducer: track " << trackID << " evt " << eventID
//<< " sec " << sec << " plane " << pla << " strip " << strip << "box
//" << box << " tube " << tub << endl;
TClonesArray& clref = *fDigiArray;
Int_t size = clref.GetEntriesFast();
return new(clref[size]) PndEmcHit(trackID, detID, energy, time, emcX[detID],
emcY[detID], emcZ[detID], mctruth);
// ----

Note that the TClonesArray is filled with an object which can have a variable vector size: mctruth! I don't know the details of TClonesArrays, but can one fill it with a dynamic array containing a std::vector <Int_t> ???? I guess that this mctruth propagation is also obsolete with the new code of Tobias. So, maybe we should remove it anyway...

Best wishes,

Previous Topic: *** glibc detected *** double free or corruption (out): 0x0a0c66f0 ***
Next Topic: Genfit server is down
Goto Forum:

Current Time: Wed Dec 06 15:19:45 CET 2023

Total time taken to generate the page: 0.00797 seconds