Secvertex amvf to be merged into main EICrecon repo #1915

bishoyDHD · 2025-06-13T18:40:41Z

Briefly, what does this PR introduce?

This adds secondary vertex finder to the 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

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

No need to make changes to existing code. Full compliance with main EICrecon repo has been maintained throughout

Does this PR change default behavior?

No change to default behaviour

… into displacedvertex

…>= 33

… into displacedvertex

veprbl · 2025-07-27T02:11:21Z

@wdconinc I know that @bishoyDHD did a good faith attempt at adapting his code to the algorithms interface. As you know, there are objective issues with interfacing ACTS_LOCAL_LOGGER with algorithms that are the same for existing CKFTracking code. The issues with passing code to functions I can't testify, but we probably can relatively easily address in further refactoring. My proposal is to review PR without asking for algorithms interface. Likely, it will be to either me or you to take on this technical debt.

wdconinc · 2025-07-27T02:28:00Z

@wdconinc I know that @bishoyDHD did a good faith attempt at adapting his code to the algorithms interface. As you know, there are objective issues with interfacing ACTS_LOCAL_LOGGER with algorithms that are the same for existing CKFTracking code. The issues with passing code to functions I can't testify, but we probably can relatively easily address in further refactoring. My proposal is to review PR without asking for algorithms interface. Likely, it will be to either me or you to take on this technical debt.

That's not the objection that's been raised. I'm ok with keeping an extra helper function that passes a spdlog logger from the factory to the algorithm, but there is no technical reason that the rest can't be using the algorithms interface.

src/algorithms/tracking/SecondaryVertexFinder.cc

veprbl · 2025-07-27T02:42:41Z

@wdconinc I know that @bishoyDHD did a good faith attempt at adapting his code to the algorithms interface. As you know, there are objective issues with interfacing ACTS_LOCAL_LOGGER with algorithms that are the same for existing CKFTracking code. The issues with passing code to functions I can't testify, but we probably can relatively easily address in further refactoring. My proposal is to review PR without asking for algorithms interface. Likely, it will be to either me or you to take on this technical debt.

That's not the objection that's been raised. I'm ok with keeping an extra helper function that passes a spdlog logger from the factory to the algorithm, but there is no technical reason that the rest can't be using the algorithms interface.

I wrote "passing code" instead of "passing pointers [to other methods of the algorithm]". My assumption is that it's a trivial change, but there is some C++ gotcha that tripped up @bishoyDHD in his attempt. I'd rather work with code that is final and effective before doing the refactoring we are talking about.

for more information, see https://pre-commit.ci

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

for more information, see https://pre-commit.ci

…finition

veprbl · 2025-07-27T18:28:47Z

Looks like we have two comments unaddressed from the first round of review. One on using spdlog, which we might punt on. And another about units of initialVariances and moving that to config.

No Clang-Tidy warnings found so I assume my comments were addressed

github-actions · 2025-07-27T19:11:42Z

Capybara summary for PR 1915

…displacedvertex

for more information, see https://pre-commit.ci

bishoyDHD · 2025-07-28T13:54:46Z

Looks like we have two comments unaddressed from the first round of review. One on using spdlog, which we might punt on. And another about units of initialVariances and moving that to config.

Moved to Config

…displacedvertex

wdconinc

Two major remaining comments from my side at this point:

We should move the creation and initialization of algorithmic structures that are event-independent into the init function, not recreate them for each event in the process function.
We should not base this algorithm on the input from ActsExamples::Trajectories, but on the input from ReconstructedParticles. That will avoid the combinatorics of the matching loop. (Even if you don't do this, the matching loop should use an early break.)

wdconinc · 2025-07-28T14:52:13Z

src/algorithms/tracking/SecondaryVertexFinderConfig.h

+  float maxDistToLinPoint          = 5.5 * Acts::UnitConstants::mm;
+  // Bin extent in z-direction
+  float spatialBinExtent         = 25 * Acts::UnitConstants::um;
+  Acts::Vector4 initialVariances = Acts::Vector4{1e+2, 1e+2, 1e+2, 1e+8};


Still needs units.

wdconinc · 2025-07-28T14:52:30Z

src/factories/tracking/SecondaryVertexFinder_factory.h

+  void ChangeRun(int32_t) {}
+


Inherit.

Suggested change

void ChangeRun(int32_t) {}

wdconinc · 2025-07-28T14:53:08Z

src/algorithms/tracking/SecondaryVertexFinderConfig.h

@@ -0,0 +1,39 @@
+#pragma once


Missing license header.

wdconinc · 2025-07-28T14:53:31Z

src/algorithms/tracking/SecondaryVertexFinder.h

+      : SecondaryVertexFinderAlgorithm{name,
+                                       {"inputReconstructedParticles", "inputActsTrajectories"},
+                                       {"outputPrimaryVertices", "outputSecondaryVertices"},
+                                       ""} {}


Missing docstring.

wdconinc · 2025-07-28T15:08:46Z

src/algorithms/tracking/SecondaryVertexFinder.cc

+    for (const auto& t : vtx.tracks()) {
+      const auto& par = vertexfinderConfigSec.extractParameters(t.originalParams);
+      m_log->trace("Track local position from vertex = {} mm, {} mm",
+                   par.localPosition().x() / Acts::UnitConstants::mm,
+                   par.localPosition().y() / Acts::UnitConstants::mm);
+      float loc_a = par.localPosition().x();
+      float loc_b = par.localPosition().y();
+
+      for (const auto& part : reconParticles) {
+        const auto& tracks = part.getTracks();
+        for (const auto& trk : tracks) {
+          const auto& traj    = trk.getTrajectory();
+          const auto& trkPars = traj.getTrackParameters();
+          for (const auto& par : trkPars) {
+            double EPSILON = std::numeric_limits<double>::epsilon(); // mm
+            if (std::abs((par.getLoc().a / edm4eic::unit::mm) - (loc_a / Acts::UnitConstants::mm)) <
+                    EPSILON &&
+                std::abs((par.getLoc().b / edm4eic::unit::mm) - (loc_b / Acts::UnitConstants::mm)) <
+                    EPSILON) {
+              m_log->trace(
+                  "From ReconParticles, track local position [Loc a, Loc b] = {} mm, {} mm",
+                  par.getLoc().a / edm4eic::unit::mm, par.getLoc().b / edm4eic::unit::mm);
+              eicvertex.addToAssociatedParticles(part);
+            } // endif
+          } // end for par
+        } // end for trk
+      } // end for part
+    } // end for t


This whole matching is not ideal, and I'd like to see it removed instead of getting into our code base and left for others to refactor later.

The input to seed finding is (only) a set of track parameters. You have these track parameters as input in the form of ReconstructedParticles. But you ALSO pass and use the ActsExamples::Trajectories (which we are actively trying to get away from...). Then after seed finding you have to run this nested set of loops for matching with ReconstructedParticles. Why not start from ReconstructedParticles to fill your input tracks? Then when you get the vertices they will point to the tracks that are filled parallel to the ReconstructedParticles vector. There will be no matching loops necessary.

We told @bishoyDHD that copy-pasting the

EICrecon/src/algorithms/tracking/IterativeVertexFinder.cc

Lines 155 to 182 in 246dae5

for (const auto& t : vtx.tracks()) {

const auto& par = finderCfg.extractParameters(t.originalParams);

m_log->trace("Track local position from vertex = {} mm, {} mm",

par.localPosition().x() / Acts::UnitConstants::mm,

par.localPosition().y() / Acts::UnitConstants::mm);

float loc_a = par.localPosition().x();

float loc_b = par.localPosition().y();

for (const auto& part : *reconParticles) {

const auto& tracks = part.getTracks();

for (const auto& trk : tracks) {

const auto& traj = trk.getTrajectory();

const auto& trkPars = traj.getTrackParameters();

for (const auto& par : trkPars) {

const double EPSILON = 1.0e-4; // mm

if (std::abs((par.getLoc().a / edm4eic::unit::mm) - (loc_a / Acts::UnitConstants::mm)) <

EPSILON &&

std::abs((par.getLoc().b / edm4eic::unit::mm) - (loc_b / Acts::UnitConstants::mm)) <

EPSILON) {

m_log->trace(

"From ReconParticles, track local position [Loc a, Loc b] = {} mm, {} mm",

par.getLoc().a / edm4eic::unit::mm, par.getLoc().b / edm4eic::unit::mm);

eicvertex.addToAssociatedParticles(part);

} // endif

} // end for par

} // end for trk

} // end for part

} // end for t

is an acceptable workaround to the issue of associating to PODIO. I don't think writing a routine to convert ReconstructedParticles to Trajectories is a useful exercise, if we plan to move to PODIO for Acts EDM anyway. Even once we have that, there will be some matching loop to have.

I don't think writing a routine to convert ReconstructedParticles to Trajectories is a useful exercise.

Not what I'm suggesting. Look at the code. Instead of getTrackParameters from Trajectories, you can getTrackParameters from ReconstructedParticles, instead of doing exactly that in the loop later.

Still not following you here. I don't see track parameters in edm4eic::ReconstructedParticle or edm4eic::Track. Or you want to match by momentum instead of locA/locB?

Or the 3D position? But then we need to convert using bound surface.

~~How can you reduce it?~~ (Messages crossed -- GitHub is lagging)

Okay, so that is about a different piece of code where the

Acts::BoundTrackParameters<> par(trkPars);

is a non-existent conversion routine from EDM4eic to Acts EDM. And that does not yet explain how to get rid of the nested match loop.

The idea is this: #1992 (not even tested compilation, and has some remaining FIXMEs).

non-existent conversion routine

In the sense that CKFTracking does this already (minus surface lookup since it's a perigee that is re-created).

I see the light. Let me see if I can make this work (I will be in the lab so please be a bit patient).
On another note. This is solution might be what I have been looking for actually, because I would like to have reconstructed particles that perhaps do not have a "good" primary vertex. Basically, let's not throwaway all of the tracks (because there could be good candidate secondary particles in there).

bishoyDHD and others added 30 commits September 2, 2024 13:38

updating secondary vertex finder

097ec32

Merge branch 'eic:main' into displacedvertex

09636b8

added reco-particles to displaced vertex

7c9162b

Merge branch 'displacedvertex' of https://github.com/bishoyDHD/EICrecon…

f109512

… into displacedvertex

Prelim secvertex finder

b6579d5

Modifying the displaced vertex finder

8063442

adding displaced vertex changes

888e5e5

Merge branch 'eic:main' into displacedvertex

4a699cb

Added more functionality to sec. vertex and now compatible with ACTS …

ee917d1

…>= 33

Merge branch 'displacedvertex' of https://github.com/bishoyDHD/EICrecon…

2f12770

… into displacedvertex

Merge branch 'eic:main' into displacedvertex

cbcf0c2

adding fuctions for sec vertex reco

fff3b03

Merge branch 'eic:main' into displacedvertex

e2e19f4

Secondary Vertexing Utility tool implentation

1d0d8a7

Updating sec vertex finder

4dd58e0

Merge branch 'eic:main' into displacedvertex

1231d7f

Merge branch 'eic:main' into displacedvertex

8f7852b

Added method to calculate the Prim. Vtx form Adaptive Multivertex Finder

4ec0b87

Updated the SecondaryVertex to calculate prim. vtx using AMVF

65b261e

cleaning up sec. vertex finder

39fdc6e

Cleaned up some std::cout printouts

4d918f5

Merge branch 'eic:main' into displacedvertex

4daa73b

added C macro for Lambda->proton pi analysis

8a5d527

Tidying up plots for Lambda decay

3436fd5

Added reco Lambda ROOT file

532a4e9

fixed some minor bugs in Lambda analysis macro

7102fa9

updating Lambda reco ROOT file

1d562f2

added runlist here

17f8ab7

Added 4th tracking dimension of time. Working properly

bfd67d5

added sec. vertex invariant mass

7a032f0

Merge branch 'main' into secvertexAMVF

93b2e50

bishoyDHD temporarily deployed to github-pages July 25, 2025 15:55 — with GitHub Actions Inactive

wdconinc reviewed Jul 27, 2025

View reviewed changes

src/algorithms/tracking/SecondaryVertexFinder.cc Outdated Show resolved Hide resolved

veprbl and others added 10 commits July 27, 2025 14:07

SecondaryVertexFinder: adopt algorithms convention for process call

d049e29

SecondaryVertexFinder: use <> includes where appropriate

1734966

add missing copyright years

6ebd198

[pre-commit.ci] auto fixes from pre-commit.com hooks

bbbcdc8

for more information, see https://pre-commit.ci

Update src/algorithms/tracking/TrackProjector.cc

c5f2c23

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into secvertexAMVF

740f6ff

avoid extra small epsilon

9119287

remove comment that's already in Config

4a00881

[pre-commit.ci] auto fixes from pre-commit.com hooks

ef1a410

for more information, see https://pre-commit.ci

SecondaryVertexFinder: avoid warnings of init shadowing its proper de…

f6f5f8d

…finition

eic deleted a comment from github-actions bot Jul 27, 2025

veprbl temporarily deployed to github-pages July 27, 2025 19:10 — with GitHub Actions Inactive

bishoyDHD and others added 3 commits July 28, 2025 09:34

Added initialVariances to Config

3cc79b9

Merge branch 'secvertexAMVF' of https://github.com/eic/EICrecon into …

fb1e371

…displacedvertex

[pre-commit.ci] auto fixes from pre-commit.com hooks

10cf069

for more information, see https://pre-commit.ci

pre-commit-ci bot temporarily deployed to github-pages July 28, 2025 14:20 Inactive

bishoyDHD added 2 commits July 28, 2025 10:31

clean up iwyu

3caae30

Merge branch 'secvertexAMVF' of https://github.com/eic/EICrecon into …

00efab5

…displacedvertex

wdconinc reviewed Jul 28, 2025

View reviewed changes

bishoyDHD temporarily deployed to github-pages July 28, 2025 15:44 — with GitHub Actions Inactive

	for (const auto& t : vtx.tracks()) {
	const auto& par = finderCfg.extractParameters(t.originalParams);
	m_log->trace("Track local position from vertex = {} mm, {} mm",
	par.localPosition().x() / Acts::UnitConstants::mm,
	par.localPosition().y() / Acts::UnitConstants::mm);
	float loc_a = par.localPosition().x();
	float loc_b = par.localPosition().y();

	for (const auto& part : *reconParticles) {
	const auto& tracks = part.getTracks();
	for (const auto& trk : tracks) {
	const auto& traj = trk.getTrajectory();
	const auto& trkPars = traj.getTrackParameters();
	for (const auto& par : trkPars) {
	const double EPSILON = 1.0e-4; // mm
	if (std::abs((par.getLoc().a / edm4eic::unit::mm) - (loc_a / Acts::UnitConstants::mm)) <
	EPSILON &&
	std::abs((par.getLoc().b / edm4eic::unit::mm) - (loc_b / Acts::UnitConstants::mm)) <
	EPSILON) {
	m_log->trace(
	"From ReconParticles, track local position [Loc a, Loc b] = {} mm, {} mm",
	par.getLoc().a / edm4eic::unit::mm, par.getLoc().b / edm4eic::unit::mm);
	eicvertex.addToAssociatedParticles(part);
	} // endif
	} // end for par
	} // end for trk
	} // end for part
	} // end for t

Secvertex amvf to be merged into main EICrecon repo #1915

Are you sure you want to change the base?

Secvertex amvf to be merged into main EICrecon repo #1915

Conversation

bishoyDHD commented Jun 13, 2025

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

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

Does this PR change default behavior?

Uh oh!

veprbl commented Jul 27, 2025

Uh oh!

wdconinc commented Jul 27, 2025

Uh oh!

Uh oh!

veprbl commented Jul 27, 2025

Uh oh!

veprbl commented Jul 27, 2025

Uh oh!

github-actions bot commented Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Capybara summary for PR 1915

Uh oh!

bishoyDHD commented Jul 28, 2025

Uh oh!

wdconinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veprbl Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bishoyDHD Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 27, 2025 •

edited

Loading

veprbl Jul 28, 2025 •

edited

Loading

bishoyDHD Jul 29, 2025 •

edited

Loading