Skip to content

Add LGADHitReconstruction that does clustering #1779

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

Open
wants to merge 58 commits into
base: main
Choose a base branch
from

Conversation

ssedd1123
Copy link
Contributor

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

TOFBarrelRecHits is now generated from the new class "LGADHitReconstruction". This class takes the digitized ADC and TDC values from each strips within the BTOF sensor to reconstruct the hit location, hit time and energy deposition.

Currently, this class is just a skeleton with position calculated by a simple weighted average, energy is just the sum of ADC values inside a sensor and time is just the earliest TDC value within a sensor, transformed linearly to get a rough time and EDep. There is no time walk correction and this class is unable to distinguish multiple hits in the same sensor if they arrive within the same EICROC cycle (25 ns). However, our group is developing a more sophisticated clustering algorithm and the time walk correction will be implemented in the future in another pull request. This is just a skeletal class for our group to develop further.

Name of the class is still open for discussion. See #1778

Does this PR change default behavior?

TOFBarrelRecHits is now generated by LGADHitReconstruction. If TOFBarrelRecHits are used by downstream classes for track reconstruction, I believe it will lower the tracking efficiency because digitization class discard hits with too low energy threshold. The lack of time walk correction and energy loss through the boundaries of BTOF sensor also contributes to the worsening of position resolution, which may or may not have huge ramifications.

Part of the reason I make this pull request is precisely because I don't fully understand it's downstream effect. Please feel free to let me know if any part of the performance is significantly degraded.

@github-actions github-actions bot added topic: PID Relates to PID reconstruction topic: barrel labels Apr 1, 2025
@simonge
Copy link
Contributor

simonge commented Apr 9, 2025

I think here you will want to split the algorithm into two parts again:

  1. Calibration of the RawTrackerHit into a TrackerHit.
  2. Clustering of the reconstructed hits into a Measurement2D.

The calibration of an individual hit can be very simple at the moment as there is no variation in the gain, delay, noise etc. from channel to channel but basic timewalk can be added there.

The TrackerHit only has a single relation to a RawTrackerHit while the Measurement2D is supposed to allow for clustering. I have a similar algorithm to what you're developing, which requires pre-divided collections, here:
https://github.com/eic/EICrecon/blob/main/src/algorithms/fardetectors/FarDetectorTrackerCluster.cc

A basic weighted clustering algorithm could easily be shared between tracking detectors but I wouldn't necessarily advise adopting mine.

This will require a bit of a change of where the Measurements are included into the rest of the tracking pipeline

@ssedd1123
Copy link
Contributor Author

I think here you will want to split the algorithm into two parts again:

  1. Calibration of the RawTrackerHit into a TrackerHit.
  2. Clustering of the reconstructed hits into a Measurement2D.

The calibration of an individual hit can be very simple at the moment as there is no variation in the gain, delay, noise etc. from channel to channel but basic timewalk can be added there.

The TrackerHit only has a single relation to a RawTrackerHit while the Measurement2D is supposed to allow for clustering. I have a similar algorithm to what you're developing, which requires pre-divided collections, here: https://github.com/eic/EICrecon/blob/main/src/algorithms/fardetectors/FarDetectorTrackerCluster.cc

A basic weighted clustering algorithm could easily be shared between tracking detectors but I wouldn't necessarily advise adopting mine.

This will require a bit of a change of where the Measurements are included into the rest of the tracking pipeline

That is a good idea. I am working on it.

Thanks for the references on clustering. Since this class is just a skeleton class for the TOF group to develop more sophisticated clustering algorithm in the future, it's better that we write a separate clustering class for BTOF.

I can certainly output the cluster information as Measurement2D, but I am not sure if the tracking group is using it. As a matter of fact, I don't know what the downstream track reconstruction class takes as input. I know that before I was asked to develop TOF class, the final output from BTOF.cc is "TOFBarrelRecHits", which is a TrackerHit class, so I just assumed that ACTS downstream takes "TOFBarrelRecHits" as input for their track reconstruction. Can you confirm if downstream classes will still work correctly if I switch "TOFBarrelRecHits" to a Measurement2D class please? Because at this stage, I do want to see how significantly track reconstruction is affacted by digitization.

@ssedd1123
Copy link
Contributor Author

ssedd1123 commented Apr 11, 2025

@simonge Is cluster.setLoc supposed to be local position? In that case, should I also use local position for the position of TrackerHit? Just want to make sure my understanding is correct before proceeding further.

Edit: Oh I get it now. "Loc" stands for local, not location.

@veprbl veprbl changed the title Pr/lgad clustering Add LGADHitReconstruction that does clustering Apr 11, 2025
@simonge
Copy link
Contributor

simonge commented Apr 11, 2025

I can certainly output the cluster information as Measurement2D, but I am not sure if the tracking group is using it. As a matter of fact, I don't know what the downstream track reconstruction class takes as input. I know that before I was asked to develop TOF class, the final output from BTOF.cc is "TOFBarrelRecHits", which is a TrackerHit class, so I just assumed that ACTS downstream takes "TOFBarrelRecHits" as input for their track reconstruction. Can you confirm if downstream classes will still work correctly if I switch "TOFBarrelRecHits" to a Measurement2D class please? Because at this stage, I do want to see how significantly track reconstruction is affacted by digitization.

The Measurement2D is the interface required by acts. At the moment all of the other TrackerHits are converted into Measurement2Ds, simply without clustering. You will need to make sure you get the correct surface from the acts geometry so it gets used correctly:
https://github.com/eic/EICrecon/blob/main/src/algorithms/tracking/TrackerMeasurementFromHits.cc

… off scale low. It could be caused by SiliconPulseDiscretization receiving more than one pulse in a cell. PulseCombiner is now enabled in BTOF.cc to hopefully eliminate this error.
@veprbl
Copy link
Member

veprbl commented Apr 24, 2025

Okay, that one benchmakr is a bit cryptic about its failures, but common cause is that something is wrong with tracking.

Looking at https://eicrecon.epic-eic.org/pr/1779/capybara/rec_dis_5x41_minQ2=0_craterlake_5x41/index.html#CentralCKFTracks
This appears to reduce number of hits on track:
image
image

@ssedd1123
Copy link
Contributor Author

A reduction in BTOF efficiency is expected because the pulse amplitude needs to pass threshold set in EICROCDigitization class. Position and timing resolution are going to get worse too because there is no time walk correction.

However, LGADHitClusterAssociation do nothing to boost the efficiency back up. All it does is to convert the Measurement2D back to TrackerHit and associate it with the corresponding raw hit, and somehow that eliminates errors from the benchmarks.

I don't know the details of that benchmark, but it seems to me that the lack of efficiency is not the only reason why that benchmark fails.

@ssedd1123
Copy link
Contributor Author

It looks like the loss of BTOF efficiency isn't that significant. I passed all tests, but only with LGADHitClusterAssociation class enabled. I can't pass the test for physics_benchmarks/results/epic_craterlake/dis/5on41/minQ2=1/kinematics_correlations without it. @simonge If I am missing a step or use Measurement2D incorrectly, please let me know.

image

@simonge
Copy link
Contributor

simonge commented May 1, 2025

From what I can see, you shouldn't need the LGADHitClusterAssociation algorithm creating TOFBarrelRecHits at all. This will just get passed into the TrackerMeasurementFromHits algorithm which does a very simple conversion back into Measurement2Ds again.

Instead you want to use the TOFBarrelClusterHits now directly.

So remove the following line to take the BTOF hits out of the other tracking workflow:
https://github.com/eic/EICrecon/blob/main/src/global/tracking/tracking.cc#L52

Then you will need to add a new Measurement2D CollectionCollector_factory which combines the other measurements with those of the BTOF before passing them on the the acts CKF tracking factories.
https://github.com/eic/EICrecon/blob/main/src/global/tracking/tracking.cc#L93

@ssedd1123
Copy link
Contributor Author

From what I can see, you shouldn't need the LGADHitClusterAssociation algorithm creating TOFBarrelRecHits at all. This will just get passed into the TrackerMeasurementFromHits algorithm which does a very simple conversion back into Measurement2Ds again.

Instead you want to use the TOFBarrelClusterHits now directly.

So remove the following line to take the BTOF hits out of the other tracking workflow: https://github.com/eic/EICrecon/blob/main/src/global/tracking/tracking.cc#L52

Then you will need to add a new Measurement2D CollectionCollector_factory which combines the other measurements with those of the BTOF before passing them on the the acts CKF tracking factories. https://github.com/eic/EICrecon/blob/main/src/global/tracking/tracking.cc#L93

I think I get it now. CollectionCollector is just collecting various TrackerHits, but since TOFBarrelRecHits is now Measurement2D, it won't work. I added a new CollectionCollector for measurement2D as you have advised. Now the tests work locally. Let's see if it passes the workflow. Thanks for the suggestions.

I renamed the output of type Measurement2D from BTOF to "TOFBarrelClusterHits". I still keep the LGADHitClusterAssociation class to generate TOFBarrelRecHits of type TrackerHit. I don't know if any other downstream algorithms need TOFBarrelRecHits of type TrackerHit. TofEfficiency_process for instance only works with TrackerHit. I keep the association class to keep things from breaking, but if we confirm there is no other side-effect when TOFBarrelRecHits is switched to type Measurement2D, then I can remove LGADHitClusterAssociation.

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

Successfully merging this pull request may close these issues.

3 participants