Skip to content

!feat: edm4eic::TrackerHit: allow many rawHits#105

Closed
wdconinc wants to merge 3 commits intomainfrom
tracker-hit-many-raw-hits
Closed

!feat: edm4eic::TrackerHit: allow many rawHits#105
wdconinc wants to merge 3 commits intomainfrom
tracker-hit-many-raw-hits

Conversation

@wdconinc
Copy link
Copy Markdown
Contributor

@wdconinc wdconinc commented Mar 24, 2025

Briefly, what does this PR introduce?

This PR modifies edm4eic::TrackerHit::rawHit to use a OneToManyRelation instead of a OneToOneRelation. This addresses a use case for the far detectors.

This is backwards incompatible since OneToManyRelations have a different set of auto-generated functions than OneToOneRelation (e.g. setRawHit -> addToRawHits). This will require that we merge this with a different major version number, and coordinate the necessary changes in EICrecon.

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 @veprbl @simonge

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

Yes. Changes required around src/algorithms/tracking/ActsToTracks.cc:198.

Does this PR change default behavior?

No.

@wdconinc wdconinc requested a review from a team as a code owner March 24, 2025 14:51
@veprbl
Copy link
Copy Markdown
Member

veprbl commented Mar 24, 2025

What is the alternative to this? We make a TrackerHitCluster?

@wdconinc
Copy link
Copy Markdown
Contributor Author

What is the alternative to this? We make a TrackerHitCluster?

I think that's overthinking the structure. In the past there has been not much interest in doing a separate clustering step in the far detectors (@simonge can correct me if that has changed).

@simonge
Copy link
Copy Markdown
Contributor

simonge commented Mar 24, 2025

I was going to suggest dropping the raw-reconstructed relation at the moment and just creating a TrackerHitSimTrackerHitLink collection. This solves the immediate problems of not being able to follow the MC information back and updating to v0.99.

I think that's overthinking the structure. In the past there has been not much interest in doing a separate clustering step in the far detectors (@simonge can correct me if that has changed).

Once charge sharing and digitization methods are implemented, clustering of the hits will be important to go back to the single position. Was this what the edm4eic::Measurement2D is meant to be anyway. What's missing is the simple 3D position and time associated with the hit to pass to a fit, acts presumably does this internally and we're never directly exposed to the points?

Is edm4hep not looking to interface with raw data at the moment? Also why have they decided not to include any timing uncertainty in the model?

If you'd like me to more fundamentally change how the Low-Q2 tracking chain is implemented to better fit the data models I'm open to suggestions.

@veprbl
Copy link
Copy Markdown
Member

veprbl commented Mar 24, 2025

Can we change FarDetectorTrackerCluster to output Measurement2D's, if those are the clusters?

@wdconinc
Copy link
Copy Markdown
Contributor Author

Measurement2D was explicitly designed for 2D measurements in the ACTS sense of pixels on planes. That is why they don't have a 3D position: it's a 2D position on a surface that is defined by the ACTS geometry ID.

Once charge sharing and digitization methods are implemented, clustering of the hits will be important to go back to the single position.

I am not sure what you will get as output from charge sharing and digitization, but if it is not a planar/time 2D+t constraint then Measurement2D is not hte right structure.

What's missing is the simple 3D position and time associated with the hit to pass to a fit

This is not within the purpose of Measurement2D. The 3D position can be derived from the 2D position and the surface. And the time is stored in Measurement2D.

Is edm4hep not looking to interface with raw data at the moment?

They are approaching this differently by necessity. They have several different types of tracker hits, which are combined through an interface type in podio. That allows creating links but not relations. Because we only have a single type of tracker hit, we can have relations.

Also why have they decided not to include any timing uncertainty in the model?

For most detectors they do not get a meaningful time uncertainty (neither do we), and they have approached this differently by allowing for a different tracker hit type for those trackers that do give precise time (with uncertainty).

Can we change FarDetectorTrackerCluster to output Measurement2D's, if those are the clusters?

If the far detectors do not use ACTS and instead just do a linear track fit to 3D coordinates, then you would be starting from the wrong structure here.

@wdconinc
Copy link
Copy Markdown
Contributor Author

I was going to suggest dropping the raw-reconstructed relation at the moment and just creating a TrackerHitSimTrackerHitLink collection.

This seems like a more complicated approach.

@ShujieL
Copy link
Copy Markdown
Contributor

ShujieL commented Mar 25, 2025

Eventually we will have clustering for tracker hits. So I think what Wouter proposed makes sense.

@simonge
Copy link
Copy Markdown
Contributor

simonge commented Mar 25, 2025

To make the workflow compatible with the Measurement2D I could just shift the conversion into global coordinates out of FarDetectorTrackerCluster and into the start of FarDetectorLinearTracking.
https://github.com/eic/EICrecon/blob/main/src/algorithms/fardetectors/FarDetectorTrackerCluster.cc#L175

This conversion could easily sit in its own algorithm which is responsible for the 2D-3D position calibration if the appropriate 3D position data type existed.

@simonge
Copy link
Copy Markdown
Contributor

simonge commented Mar 25, 2025

I am not sure what you will get as output from charge sharing and digitization, but if it is not a planar/time 2D+t constraint then Measurement2D is not hte right structure.

At least initially this is all I would intend to get out of the charge sharing and re-clustering. Maybe at some point a calibrated energy in addition, which may not just be the sum of the individual TrackerHit energies, which could currently be stored in the weights.

Even further down the line we might be able to get some PID (gamma/mip) and very limited momentum information out of the charge sharing if an appropriate model is used.

@veprbl
Copy link
Copy Markdown
Member

veprbl commented Mar 26, 2025

Looks like this is now resolved by eic/EICrecon#1770

@veprbl veprbl closed this Mar 26, 2025
@veprbl veprbl deleted the tracker-hit-many-raw-hits branch March 26, 2025 20:29
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 this pull request may close these issues.

4 participants