Fix calorimetry#819
Conversation
| length = 0 | ||
| # If the particle is a track in the MMCTrackList take the | ||
| # energy at the entrance and exit of the hull. | ||
| # NOTE: We do not consider daughters of tracks, |
There was a problem hiding this comment.
Is this correct? If the track has daughters, then my understanding is that these would be generated at the end of the track (I don't think there is any particle generation without "ending" the current particle). We would then have to check the energy at the end of the track and then also consider the daughters of the track. Also if you aren't specifically removing the daughters of the track aren't these going to show up either in other tracks or in the cascade particles?
There was a problem hiding this comment.
The problem lies with the e_ent_track variable, which represents the energy of a track at its first intersection with the surface. Therefore, this energy already encompasses all the energies of its potential daughters. If one were to continue to loop through the daughters of this track, one would again count energies that are already captured in e_ent_track. I agree that this is not the most sensible handling of this case. Especially in events like a double bang, one would essentially count the energy of the second cascade towards the track.
There was a problem hiding this comment.
However, it does not take into account daughters of tracks, since the recursion is stopped once there is a track.
Note that in the case a track is registered, the function does not call the next recursion like it's done in other cases, e.g. the call here:
graphnet/src/graphnet/data/extractors/icecube/i3calorimetry.py
Lines 253 to 266 in 93d7f45
There was a problem hiding this comment.
Is it possible that we should just drop the e_ent_track, which is essentially the energy of all tracks as they enter the detector volume, I don't currently use it at and just created it since a similar feature exists in I3HighestEparticleExtractor The actual value of this quantity might be of limited use and also does not fit great with it being a calorimetric approach.
There was a problem hiding this comment.
But it is used to calculate the e_total, which is arguably the most important quantity in this extractor.
There was a problem hiding this comment.
There was a problem hiding this comment.
We could switch out e_ent_track with e_dep_track
There was a problem hiding this comment.
Yeah, this target task is of course easier than the previous one, but it is more aligned with the calorimetric approach. Otherwise we would have to think a bit more about how to calculate this e_ent_track in a meaningful way.
There was a problem hiding this comment.
Yeah, I agree with you. It is a more calorimetric way if we only consider the deposited energy. I did some checks on the e_visible calculated with the current status of this PR from the calorimetry extractor, and it seemed to be a nice label to train on. I am looking more into the HEP now to see if it is a good substitute for this. If yes, I'm happy to get rid of e_ent_track in the calorimetry extractor.
There was a problem hiding this comment.
I have thought about it a bit more, and there is an extra amount of work that is implied when also looking at the daughters of tracks. The problem is that the daughters of tracks frequently are track segments, which are not physical particles, which implies that double-counting energies again can happen quite easily. Frankly, I do not have the time to look into this more. I think this PR already fixes the Bugs mentioned in issue #816, and I would leave it at that for the moment. We could mention this PR in the class description to make the user aware of the limitations that exist for now. Do you agree with this @Aske-Rosted?
|
I am also interested in knowing whether these changes significantly impact the running time or if it is roughly the same, did you do any testing of this? |
I have not made any tests for this yet |
7a6d1c7 to
93d7f45
Compare
Aske-Rosted
left a comment
There was a problem hiding this comment.
This looks good to me now, however one of the tests seems to be failing after your latest push.
|
That was just a connection error to the ERDA-hosted services. It should be all good now. |
This PR is addressing issue #816.
The modifications consist of:
Updates in the call function:
get_energies). This assures that nothing is counted twice.e_totalis smaller than the primary energies always, not only if daughters is True.e_totalshould never exceed the summed primary particles.primarieslist is empty (insert nan)EDIT:
New Feature
cascade_deposited_onlywhich allows to count the true energies of cascades, not only the visible ones