-
Notifications
You must be signed in to change notification settings - Fork 23
PF hit association #1856
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
base: trunk
Are you sure you want to change the base?
PF hit association #1856
Conversation
tomeichlersmith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, I have a few cleanup requests and one comment about sharing EcalGeo information with CLUE.
Remove remnant of attempt to set hits directly Co-authored-by: Tom Eichlersmith <31970302+tomeichlersmith@users.noreply.github.com>
Co-authored-by: Tom Eichlersmith <31970302+tomeichlersmith@users.noreply.github.com>
|
The failing histogram tests are caused by a name change |
| std::vector<double> layer_thickness = {2., 3.5, 5.3, 5.3, 5.3, 5.3, | ||
| 5.3, 5.3, 5.3, 5.3, 5.3, 10.5, | ||
| 10.5, 10.5, 10.5, 10.5}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true for v15 anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's right, and tom pointed out that we should look this up instead of hardwiring it. i agree, and, i didn't introduce this part myself and don't really have time to work on it in the near future. would you be ok with making a new issue for this instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, makes sense!
| Examples | ||
| -------- | ||
| from LDMX.Recon.pfReco import pileupFinder | ||
| p.sequence.append( pileupFinder ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add this to the 2e CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to do this without also developing a corresponding DQM analyzer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it still makes a technical check on it, we can see how the timing changes, etc. The DQM should anyway be it's own processor that we can add whenever it's developed
I am updating ldmx-sw, here are the details.
What are the issues that this addresses?
This allows association back to hits from clusters that were associated with PF tracks in the PFlow algorithm. It also includes a first go at a pileup electron identifier processor, that subtracts hits associated to high-momentum tracks from the ecal rechit collection (or rather, makes a new collection without those hits).
Check List
Plot showing 10 2e inclusive events, ecal rec hits x: black is all hits, red is after pileup removal. This is not at all optimized yet (clustering needs improvements) but the principle works.