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

Add association between raw tracker/calorimeter hit and tracker/calorimeter hit #86

Merged
merged 12 commits into from
Jul 26, 2024

Conversation

rahmans1
Copy link
Contributor

Briefly, what does this PR introduce?

Add association between raw tracker hit and tracker hit to have full association path from MCParticle to tracker hit.

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?

Does this PR change default behavior?

edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
wdconinc
wdconinc previously approved these changes Jul 26, 2024
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I think this adds the remaining links at the input of the tracker and calorimeter chains.

We discussed this yesterday and we think there is no need for a weight in the TrackerHit and CalorimeterHit, nor is there a need for a OneToManyRelations in these data types.

wdconinc
wdconinc previously approved these changes Jul 26, 2024
veprbl
veprbl previously approved these changes Jul 26, 2024
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discussed quite extensively previously in reco meetings. The change looks good to me.

@veprbl veprbl requested a review from mdiefent July 26, 2024 17:41
ruse-traveler
ruse-traveler previously approved these changes Jul 26, 2024
Copy link

@ruse-traveler ruse-traveler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@rahmans1 rahmans1 dismissed stale reviews from ruse-traveler and veprbl via a0eb84c July 26, 2024 17:49
@rahmans1 rahmans1 requested review from veprbl and wdconinc July 26, 2024 17:49
@rahmans1 rahmans1 changed the title Add association between raw tracker hit and tracker hit Add association between raw tracker/calorimeter hit and tracker/calorimeter hit Jul 26, 2024
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backwards compatible change.

@rahmans1 rahmans1 enabled auto-merge (squash) July 26, 2024 20:38
@rahmans1 rahmans1 merged commit a4caada into main Jul 26, 2024
3 checks passed
@rahmans1 rahmans1 deleted the rahmans1-patch-1 branch July 26, 2024 20:42
github-merge-queue bot pushed a commit to eic/EICrecon that referenced this pull request Aug 7, 2024
…ucted tracker/calorimeter hit (#1535)

### Briefly, what does this PR introduce?
Add one-to-one relation with raw tracker/calorimeter hit for
reconstructed tracker/calorimeter hit. Contingent on
eic/EDM4eic#86.

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

### Does this PR change default behavior?

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>
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