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

LCIO: Add setting of ProducedBySecondary bit for SimTrackerHits #145

Merged
merged 2 commits into from Apr 20, 2017

Conversation

Projects
None yet
4 participants
@andresailer
Member

andresailer commented Apr 7, 2017

if the hit is produced by a particle that is not kept in the MCParticle history we set the bit

added oid (original ID) to Geant4Particle class for faster lookup, as this ID is stored with the hit
Otherwise we need a complicated and time-consuming logic to parse the equivalent map if this hit really belonged to that track

Needs iLCSoft/LCIO#21 to work, but compiles also against older LCIO versions

BEGINRELEASENOTES

  • LCIOOutput: Add setting of ProducedBySecondary bit for SimTrackerHits if the hit is produced by a particle that is not stored in the MCParticle collection, needs lcio 2.8

ENDRELEASENOTES

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 7, 2017

For #126

@petricm

This comment has been minimized.

Member

petricm commented Apr 19, 2017

@gaede @andresailer shouldn't this be in the last tag?

@gaede

This comment has been minimized.

Contributor

gaede commented Apr 19, 2017

I guess no one has ever reviewed/merged this, so it has not been included in any (pre-)release tag ...

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 19, 2017

I was hoping @MarkusFrankATcernch would have a look as I am changing one of the main classes for DDG4.

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Apr 19, 2017

This is the solution to a symptom. The real issue is deeper.
In principle this feature should be extracted in an event action and saved in the Geant4Particle object
as a flag. This lookup in the particle map for the original G4Track-id should be done centrally.
At the time of the lookup it is also clear if the hit was produced by a secondary or not.

What is happening here is, that every output action (LCIO, ROOT,....) effectively must do this lookup explicitly and setting the options. This in the long run will not help, but rather increase confusion.
I suggest we separate this functionality into an event action and then also the output actions will become simpler.

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 19, 2017

I don't think I understand what you want to store where, and which things to do centrally?
What do you want to store in the Geant4Particle? Or do you mean the CalorimeterHit object?

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Apr 19, 2017

The hits typically must be patched. This is code similar to this (here ROOT output):

        for(size_t i=0; i<nhits; ++i)   {
          Geant4HitData* h = coll->hit(i);
          Geant4Tracker::Hit* trk_hit = dynamic_cast<Geant4Tracker::Hit*>(h);
          if ( 0 != trk_hit )   {
            Geant4HitData::Contribution& t = trk_hit->truth;
            int trackID = t.trackID;
            t.trackID = m_truth->particleID(trackID);
          }
          Geant4Calorimeter::Hit* cal_hit = dynamic_cast<Geant4Calorimeter::Hit*>(h);
          if ( 0 != cal_hit )   {
            Geant4HitData::Contributions& c = cal_hit->truth;
            for(Geant4HitData::Contributions::iterator j=c.begin(); j!=c.end(); ++j)  {
              Geant4HitData::Contribution& t = *j;
              int trackID = t.trackID;
              t.trackID = m_truth->particleID(trackID);
            }
          }

Take the last 4 lines:
here, if t.trackID != trackID ==> hit from secondary
I think this is correct.
Now this should be handled centrally.

For LCIO it is similar, except that the lookup is not intrusive as for ROOT, where parallel streams are currently not possible, since the above code may not be executed twice.
Or do I misunderstand the problem?

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 19, 2017

t.trackID != trackID ==> hit from secondary

This is what I tried in the beginning. But this would only work if you patch the trackID that belongs to the hits. (is that what your first sentence refers to?)
The hits have the originalTrackID (which I want to add to the Geant4Particle). The originalTrackID is not the same as the trackID, even for particles that are kept, because the rebaseParticles changes the trackID used in the m_truth->particleID map.

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Apr 19, 2017

OK. I understand now.
Then your patch is fine. But please do not call it oid. Nobody will remember what it really means.
Call it something more descriptive: g4ID, g4id, etc. similar to g4Parent which is the G4 parent's id....

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 19, 2017

I would use originalTrackID?

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Apr 19, 2017

originalG4ID or g4OriginalID ?

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 19, 2017

Sold!

LCIO: Add setting of ProducedBySecondary bit for SimTrackerHits
if the hit is produced by a particle that is not kept in the MCParticle history we set the bit
added originalG4ID to Geant4Particle class for faster lookup, as this ID is stored with the hit
Otherwise we need a complicated and time-consuming logic to parse the equivalent map if this hit really belonged to that track

@andresailer andresailer force-pushed the andresailer:addQuality branch from 831abe3 to a5ade9a Apr 19, 2017

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 19, 2017

I already change to originalG4ID

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Apr 19, 2017

Forgot to add: This should NOT be ROOT persistent (has no meaning afterwards)

Hence declare it like this:
int originalG4ID; //! not persistent

see "ref" as an example....

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 19, 2017

Should I change the order of the members? Does it have to be its own statement?

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Apr 19, 2017

It needs to be a separate statement. Just split the lines....

@andresailer

This comment has been minimized.

Member

andresailer commented Apr 20, 2017

add the not persistent comment

@andresailer andresailer merged commit 5c22733 into AIDASoft:master Apr 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@andresailer andresailer deleted the andresailer:addQuality branch Apr 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment