Skip to content

Subdet track fits ii #4263

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.

Sign up for GitHub

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
gregottino wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Subdet track fits ii #4263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
755d1d4
update to fit Si or TPC only tracks with track pruner
May 7, 2026
6e22dc7
restoring vertex finder to current master
May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions offline/packages/trackbase/ActsSurfaceMaps.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ bool ActsSurfaceMaps::isTpcSurface(const Acts::Surface* surface) const
return m_tpcVolumeIds.find(surface->geometryId().volume()) != m_tpcVolumeIds.end();
}

bool ActsSurfaceMaps::isSiSurface(const Acts::Surface* surface) const
{
return m_siVolumeIds.find(surface->geometryId().volume()) != m_siVolumeIds.end();
}

bool ActsSurfaceMaps::isMicromegasSurface(const Acts::Surface* surface) const
{
return m_micromegasVolumeIds.find(surface->geometryId().volume()) != m_micromegasVolumeIds.end();
Expand Down
7 changes: 7 additions & 0 deletions offline/packages/trackbase/ActsSurfaceMaps.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ struct ActsSurfaceMaps
//! true if given surface corresponds to TPC
bool isTpcSurface(const Acts::Surface* surface) const;

//! true if given surface corresponds to the silicon
bool isSiSurface(const Acts::Surface* surface) const;

//! true if given surface corresponds to Micromegas
bool isMicromegasSurface(const Acts::Surface* surface) const;

Expand Down Expand Up @@ -65,6 +68,10 @@ struct ActsSurfaceMaps
/** it is used to quickly tell if a given Acts Surface belongs to the TPC */
std::set<int> m_tpcVolumeIds;

//! stores all acts volume ids relevant to the Silicon
/** it is used to quickly tell if a given Acts Surface belongs to the Silicon */
std::set<int> m_siVolumeIds;

//! stores all acts volume ids relevant to the micromegas
/** it is used to quickly tell if a given Acts Surface belongs to micromegas */
std::set<int> m_micromegasVolumeIds;
Expand Down
6 changes: 6 additions & 0 deletions offline/packages/trackreco/MakeActsGeometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,12 @@ int MakeActsGeometry::InitRun(PHCompositeNode *topNode)
}
}

// fill Si volume ids
for (const auto &[hitsetid, surface] : m_clusterSurfaceMapSilicon)
{
surfMaps.m_siVolumeIds.insert(surface->geometryId().volume());
}

// fill Micromegas volume ids
for (const auto &[hitsetid, surface] : m_clusterSurfaceMapMmEdit)
{
Expand Down
37 changes: 29 additions & 8 deletions offline/packages/trackreco/PHActsTrkFitter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,11 +526,11 @@ void PHActsTrkFitter::loopTracks(Acts::Logging::Level logLevel)

// position comes from the silicon seed, unless there is no silicon seed
Acts::Vector3 position(0, 0, 0);
if (siseed)
if (siseed && !m_ignoreSilicon)
{
position = TrackSeedHelper::get_xyz(siseed) * Acts::UnitConstants::cm;
}
if (!siseed || !is_valid(position) || m_ignoreSilicon)
if (!siseed || !is_valid(position) || m_forceTpcOnlyFit)
{
position = TrackSeedHelper::get_xyz(tpcseed) * Acts::UnitConstants::cm;
}
Comment on lines +529 to 536
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Position regression when m_ignoreSilicon=true: seed perigee is created at the origin instead of the TPC seed position.

When m_ignoreSilicon=true, siseed is non-null, and m_forceTpcOnlyFit=false:

  • Line 529 — if (siseed && !m_ignoreSilicon) is false → position stays (0, 0, 0).
  • Line 533 — !siseed is false, !is_valid((0,0,0)) is false (no NaN), m_forceTpcOnlyFit is false → the TPC fallback is never reached.

The Acts::PerigeeSurface (line 695) and the four-vector seed (line 697) are then anchored at the origin, which can cause propagation failures or severely biased fits for any run using ignoreSilicon().

The !m_ignoreSilicon guard was correctly added to line 529 to stop seeding from silicon when its clusters are excluded, but the corresponding fallback on line 533 was not extended to cover that case.

🔧 Proposed fix 
-      if (!siseed || !is_valid(position) || m_forceTpcOnlyFit)
+      if (!siseed || !is_valid(position) || m_ignoreSilicon || m_forceTpcOnlyFit)

Or, equivalently, restructure as a single conditional:

-      if (siseed && !m_ignoreSilicon)
-      {
-        position = TrackSeedHelper::get_xyz(siseed) * Acts::UnitConstants::cm;
-      }
-      if (!siseed || !is_valid(position) || m_forceTpcOnlyFit)
-      {
-        position = TrackSeedHelper::get_xyz(tpcseed) * Acts::UnitConstants::cm;
-      }
+      if (siseed && !m_ignoreSilicon && !m_forceTpcOnlyFit)
+      {
+        position = TrackSeedHelper::get_xyz(siseed) * Acts::UnitConstants::cm;
+      }
+      if (!is_valid(position))
+      {
+        position = TrackSeedHelper::get_xyz(tpcseed) * Acts::UnitConstants::cm;
+      }

Note: the restructured form always falls back to the TPC position for any uncovered case (no silicon, silicon ignored, TPC-only mode, or invalid position).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (siseed && !m_ignoreSilicon)
{
position = TrackSeedHelper::get_xyz(siseed) * Acts::UnitConstants::cm;
}
if (!siseed || !is_valid(position) || m_ignoreSilicon)
if (!siseed || !is_valid(position) || m_forceTpcOnlyFit)
{
position = TrackSeedHelper::get_xyz(tpcseed) * Acts::UnitConstants::cm;
}
if (siseed && !m_ignoreSilicon)
{
position = TrackSeedHelper::get_xyz(siseed) * Acts::UnitConstants::cm;
}
if (!siseed || !is_valid(position) || m_ignoreSilicon || m_forceTpcOnlyFit)
{
position = TrackSeedHelper::get_xyz(tpcseed) * Acts::UnitConstants::cm;
}

Expand Down Expand Up @@ -574,6 +574,13 @@ void PHActsTrkFitter::loopTracks(Acts::Logging::Level logLevel)
continue;
}
}
//else if (m_forceTpcOnlyFit)
//{
// if (surface_apr->geometryId().volume() < 14)
// {
// continue;
// }
//}
bool pop_flag = false;
if (surface_apr->geometryId().approach() == 1)
{
Expand Down Expand Up @@ -643,7 +650,7 @@ void PHActsTrkFitter::loopTracks(Acts::Logging::Level logLevel)
float seedphi = 0;
float seedtheta = 0;
float seedeta = 0;
if (siseed)
if (siseed && !m_forceTpcOnlyFit)
{
seedphi = siseed->get_phi();
seedtheta = siseed->get_theta();
Expand Down Expand Up @@ -984,6 +991,9 @@ SourceLinkVec PHActsTrkFitter::filterSourceLinks(const SourceLinkVec& sourceLink
if (m_forceSiOnlyFit && (m_tGeometry->maps().isMicromegasSurface(surf) || m_tGeometry->maps().isTpcSurface(surf)) )
{ continue; }

if (m_forceTpcOnlyFit && (m_tGeometry->maps().isMicromegasSurface(surf) || m_tGeometry->maps().isSiSurface(surf)) )
Copy link
Copy Markdown
Contributor

@osbornjd osbornjd May 10, 2026

Choose a reason for hiding this comment

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

This seems to be only skipping the non TPC surfaces, but the source links are still added right?

Copy link
Copy Markdown
Contributor Author

@gregottino gregottino May 11, 2026

Choose a reason for hiding this comment

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

Yeah I didn't try and skip adding sourcelinks, I just ignored them in the fitting process.

Copy link
Copy Markdown
Contributor

@osbornjd osbornjd May 16, 2026

Choose a reason for hiding this comment

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

That means this will only work for the direct navigator then, I think

{ continue; }

// update vectors
filtered.push_back(sl);
}
Expand Down Expand Up @@ -1070,7 +1080,7 @@ void PHActsTrkFitter::updateSvtxTrack(
track->identify();
}

if (!m_fitSiliconMMs && !m_forceSiOnlyFit)
if (!m_fitSiliconMMs && !m_forceSiOnlyFit && !m_forceTpcOnlyFit)
{
track->clear_states();
}
Expand All @@ -1097,9 +1107,20 @@ void PHActsTrkFitter::updateSvtxTrack(
track->set_y(params.position(m_transient_geocontext)(1) / Acts::UnitConstants::cm);
track->set_z(params.position(m_transient_geocontext)(2) / Acts::UnitConstants::cm);

track->set_px(params.momentum()(0));
track->set_py(params.momentum()(1));
track->set_pz(params.momentum()(2));
auto* seed = track->get_tpc_seed();

if(!m_forceSiOnlyFit)
{
track->set_px(params.momentum()(0));
track->set_py(params.momentum()(1));
track->set_pz(params.momentum()(2));
}
else
{
track->set_px(seed->get_px());
Copy link
Copy Markdown
Contributor

@osbornjd osbornjd May 10, 2026

Choose a reason for hiding this comment

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

I'm not sure I understand this, why would we set the output track to have momentum from the seed parameters?

Copy link
Copy Markdown
Contributor Author

@gregottino gregottino May 11, 2026

Choose a reason for hiding this comment

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

In the case of the Si only track fit, the pT resolution is so bad it actually is a huge improvement to use the TPC seed pT for the final value of pT instead of the Si fitted pT.

Copy link
Copy Markdown
Contributor

@osbornjd osbornjd May 16, 2026

Choose a reason for hiding this comment

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

So what happens if the silicon-TPC match was wrong? I'm not sure we can just unilaterally do this for all attempted track fits

track->set_py(seed->get_py());
track->set_pz(seed->get_pz());
}

track->set_charge(params.charge());
track->set_chisq(trajState.chi2Sum);
Expand Down Expand Up @@ -1134,7 +1155,7 @@ void PHActsTrkFitter::updateSvtxTrack(

// in using silicon mm fit also extrapolate track parameters to all TPC surfaces with clusters
// get all tpc clusters
auto* seed = track->get_tpc_seed();

if (m_fitSiliconMMs && seed)
{
// acts propagator
Expand Down
6 changes: 6 additions & 0 deletions offline/packages/trackreco/PHActsTrkFitter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ class PHActsTrkFitter : public SubsysReco
m_forceSiOnlyFit = forceSiOnlyFit;
}

void forceTpcOnlyFit(bool forceTpcOnlyFit)
{
m_forceTpcOnlyFit = forceTpcOnlyFit;
}

/// require micromegas in SiliconMM fits
void setUseMicromegas(bool value)
{
Expand Down Expand Up @@ -217,6 +222,7 @@ class PHActsTrkFitter : public SubsysReco
bool m_fitSiliconMMs = false;

bool m_forceSiOnlyFit = false;
bool m_forceTpcOnlyFit = false;

/// requires micromegas present when fitting silicon-MM surfaces
bool m_useMicromegas = true;
Expand Down