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.

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

ReconstructedChargedParticlesAssociations with too-often missing associations #450

Closed
KongTu opened this issue Jan 27, 2023 · 11 comments · Fixed by #496
Closed

ReconstructedChargedParticlesAssociations with too-often missing associations #450

KongTu opened this issue Jan 27, 2023 · 11 comments · Fixed by #496
Assignees

Comments

@KongTu
Copy link

KongTu commented Jan 27, 2023

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
Copy link
Contributor

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

@wdconinc
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

wdconinc commented Feb 8, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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

@fbossu
Copy link

fbossu commented Feb 14, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants