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

IterativeVertexFinder: fill vertex-to-particle relation #1576

Merged
merged 22 commits into from
Aug 26, 2024
Merged

Conversation

starsdong
Copy link
Contributor

@starsdong starsdong commented Aug 16, 2024

Briefly, what does this PR introduce?

  • added association for ReconstructionParticles to output vertices

What kind of change does this PR introduce?

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?

  • ReconstructedParticle association added to the default output CKFCentralVertices

- added association for ReconstructionParticles to output vertices
@github-actions github-actions bot added the topic: tracking Relates to tracking reconstruction label Aug 16, 2024
src/algorithms/tracking/IterativeVertexFinder.cc Outdated Show resolved Hide resolved
src/algorithms/tracking/IterativeVertexFinder.cc Outdated Show resolved Hide resolved
src/algorithms/tracking/IterativeVertexFinder.cc Outdated Show resolved Hide resolved
src/algorithms/tracking/IterativeVertexFinder.cc Outdated Show resolved Hide resolved
src/global/tracking/tracking.cc Outdated Show resolved Hide resolved
src/global/tracking/IterativeVertexFinder_factory.h Outdated Show resolved Hide resolved
Copy link

github-merge-queue bot pushed a commit that referenced this pull request Aug 23, 2024
The issues with rendering the factory graph are cropping in. Example
PRs: #1577 #1576
@veprbl veprbl removed their assignment Aug 23, 2024
@veprbl
Copy link
Member

veprbl commented Aug 23, 2024

image
Associations are filled.

veprbl
veprbl previously approved these changes Aug 23, 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.

LGTM

@veprbl veprbl changed the title Update in IterativeVertexFinder IterativeVertexFinder: fill vertex-to-particle relation Aug 23, 2024
@veprbl veprbl enabled auto-merge August 25, 2024 02:27
@starsdong
Copy link
Contributor Author

Hi Woulter and Dmitry,

Are these anything you would like to comment before merging this PR? The requested change Woulter brought up, I believe, is addressed in the later commits. Would appreciate if you can take a review to see whether we can proceed. Thanks

/xin

@veprbl
Copy link
Member

veprbl commented Aug 26, 2024

I don't understand why this hasn't merged over the weekend. Maybe we needed to have and approval form @wdconinc or dismiss his "1 change requested" review.

@veprbl veprbl mentioned this pull request Aug 26, 2024
7 tasks
wdconinc
wdconinc previously approved these changes Aug 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.

The 'request changes' is always sticky and requires the same person who requested changes to give explicit approval. In this case, I requested changes since we needed to use the podio collection instead of std::vector. That's been done now, so I approve.

One more comment though on a magic number that should be a configuration variable (best) or defined as a ratio of another dimensionful quantity. The reconstruction should not care whether we work in dd4hep units or, for some reason, another unit system.

@veprbl veprbl dismissed stale reviews from wdconinc and themself via cdeaf5b August 26, 2024 19:35
Copy link

@veprbl veprbl added this pull request to the merge queue Aug 26, 2024
Merged via the queue into main with commit 816a8da Aug 26, 2024
86 of 87 checks passed
@veprbl veprbl deleted the pr/ivf_assoc branch August 26, 2024 20:20
@veprbl veprbl added this to the 24.09.0 milestone Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure topic: tracking Relates to tracking reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants