eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
6 stars 27 forks source link

ReconstructedChargedParticlesAssociations with too-often missing associations #450

Closed KongTu closed 1 year ago

KongTu commented 1 year ago

Environment: (where does this bug occur, have you tried other environments)

This was found in the EICrecon output from the Oct simulation campaign.

Steps to reproduce: (give a step by step account of how to trigger the bug)

  1. git clone https://github.com/KongTu/EICreconOutputReader.git
  2. cd EICreconOutputReader
  3. ./runTestAssociation.sh input/input-eicrecon-DIS_18x275_Q2-1_0001.root eicrecon-DIS_18x275_Q2-1_0001

Expected Result: (what do you expect when you execute the steps above)

I expect that for reconstructed scattered electron, it should almost always have an association to the MC.

Actual Result: (what do you get when you execute the steps above)

the printout will show events that a reconstructed level scattered electron is found, no association can be found, meaning the recID is not in the ReconstructedChargedParticlesAssociations. The issue here seems to me that it happens too often.

wdconinc commented 1 year ago

IMHO The first step in an issue really should not be a git clone of another repository...

wdconinc commented 1 year ago

Can we not make this a bit narrower? Where does that input file even come from? Is this arches or brycecanyon? The file is 2 months old. Are we surprised about missing associations when the clustering was not working? Is this an issue that is present in the current version of EICrecon?

bspage912 commented 1 year ago

I will go into more detail and post plots later (don't have time to put slides together before I head home), but I think Kong's point about cutting on the momentum match is correct. I went through and by hand geometrically matched all reconstructed tracks which did not have an associated id to an MC particle. In the vast majority of cases, the reco/mc deltaR is as good as the reco tracks with an association, but the relative difference in momentum between reco and mc is greater than 10%. This 10% really looks like a hard threshold used to determine if there is a match or not as it really is basically all associated tracks have a delta momentum < 10% and all unassociated tracks have a delta momentum > 10%.

If this is the case, I think we should switch to doing the association purely geometrically, as now we are ignoring the tracks with the most pathological momentum reconstruction if the analysis is done using the associatedParticles branch.

wdconinc commented 1 year ago

Can we get an agreement among all PWGs on what the momentum matching distance should be? The algorithm is essentially these three lines: https://github.com/eic/EICrecon/blob/main/src/algorithms/tracking/ParticlesWithTruthPID.cpp#L97-L99

The default for momentum is indeed 10%, https://github.com/eic/EICrecon/blob/main/src/algorithms/tracking/ParticlesWithTruthPIDConfig.h#L11

There is an opportunity for someone to run a study here to inform this choice.

bspage912 commented 1 year ago

I would again argue that relative momentum distance should not be used for reco-mc matching. This just raises the spectre of placing cuts on a quantity you want to measure (and yes, I know purely geometric matching has implications for angular resolution studies). This actually came up in today's Inclusive group meeting in the context of kinematic reconstruction and there was a question about why the track momentum resolution seemed to have hard cuts at 10%. In this case, they even did the recochargedparticle to mc matching by hand, but there was a cut on r.c.p PID which brings in the association algorithm. This would have been very non-obvious if I had not been serendipitously looking at the same issue.

We should definitely ping the working groups to see what acceptable limits would be, but also just to make sure everyone is aware of the way matching works now. I can try to do some systematic studies looking at track/mc associations, but they will have to take a back seat to other work I'm doing for the pfRICH. I have seen examples of tracks which should be matched to particles according to the EICrecon algorithm but don't seem to be, so this will require more investigation.

Maybe optimising this algo is something for the tracking wg to look into as they may be able to look at some more fundamental tracking properties than I am able to...

wdconinc commented 1 year ago

Re-triggering this. We need a propposal to get consensus on before we submit large campaigns.

Current status:

    struct ParticlesWithTruthPIDConfig {
        double momentumRelativeTolerance = 0.1;    /// Matching momentum tolerance requires 10% by default;
        double phiTolerance = 0.030;        /// Matching phi tolerance [mrad]
        double etaTolerance = 0.2;          /// Matching eta tolerance of 0.2
    };

Proposal (easiest without code changes):

    struct ParticlesWithTruthPIDConfig {
        double momentumRelativeTolerance = 100.0;    /// Matching momentum effectively disabled
        double phiTolerance = 0.030;        /// Matching phi tolerance [mrad]
        double etaTolerance = 0.2;          /// Matching eta tolerance of 0.2
    };

This will allow abs(dp/p_mc) up to 100, and effectively disable it.

Do the other parameters sound good? If there are multiple matches, the best one in a weighted sum of squares will be used. The momentumRelativeTolerance will also reduce the weight of the momentum difference in this sum.

bspage912 commented 1 year ago

Hey Wouter, this seems reasonable to me. The phi tolerance may be a little tight - could probably go to 0.1, I've done some quick studies on this and will try to put a few slides together tomorrow that can go out to the tracking group list for comment.

I have also observed a small fraction of 'duplicate reconstructed tracks' - pairs of reconstructed tracks with basically identical momenta, eta, and phi. I suppose this is probably to be expected at some level. I want to quantify this a bit more and I will again report to tracking

bspage912 commented 1 year ago

Sent out a note to the tracking list with a summary of my studies (attached here as well) trackParticleMatching.pdf

fbossu commented 1 year ago

Hi, as I replied to Brian in the mail thread, I think that at some point we should move away from this phase space matching and prefer a hit based matching.

wdconinc commented 1 year ago

prefer a hit based matching

Yes. At some point we should propagate the associations that makes all this unnecessary. We start off with hits each cleanly associated with a single MC particle. These form tracks or clusters that should each be associated with an MC particle (possibly weighted). The problem here, I think, is that we lose the associations in the tracking algorithm... We are just doing a very adhoc matching to make up for that.

bspage912 commented 1 year ago

Hi All, I presented my study on this issue to the tracking working group today. Didn't get any objections to negating the relative momentum difference cut / inclusion in the deltaR calculation. Also no strong opinions on relaxing dphi condition from 0.03 to 0.1 (Xuan suggested doing only for high eta, but I don't think we have to make this distinction).

I propose we just make the changes and close this issue.

Incidentally, how do users know what software changes go into the different simulation designations (23.02.xx etc)?