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

feat!: Updates to GBTS for Athena Implementation #3882

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
b82ec02
Adding comment
Rosie-Hasan Sep 23, 2024
86dff84
adding new r method
Rosie-Hasan Sep 6, 2024
d7946d4
fixing semicolon
Rosie-Hasan Sep 6, 2024
9c4433a
changing use of source links in core
Rosie-Hasan Oct 7, 2024
3730c4a
Adding option to input to cluster width
Rosie-Hasan Oct 15, 2024
0566551
fixing phi mistake
Rosie-Hasan Nov 13, 2024
41661f1
changing file name to CamelCase
Rosie-Hasan Nov 19, 2024
7d79244
realised seedfilter object never used
Rosie-Hasan Nov 19, 2024
759b72e
got rid of unused parameter
Rosie-Hasan Nov 19, 2024
43eb185
removing commented lines
Rosie-Hasan Nov 19, 2024
7c96cd1
removing commented lines
Rosie-Hasan Nov 20, 2024
9ddf2e4
fixing spelling in GbtsSeedingAlgorithm.cpp
Rosie-Hasan Nov 21, 2024
b33faca
fixing gbts data storage
Rosie-Hasan Nov 22, 2024
35c79c3
Merge branch 'main' into UpdatesForAthenaOnMain
Rosie-Hasan Dec 11, 2024
1a7b3a0
running clang format
Rosie-Hasan Dec 11, 2024
8347b8a
github CI requested different formatting to my local clang format
Rosie-Hasan Dec 11, 2024
ac89d45
Update SeedFinderGbts.ipp
Rosie-Hasan Jan 14, 2025
0bff8e8
Update Core/include/Acts/Seeding/GbtsDataStorage.hpp
Rosie-Hasan Jan 14, 2025
7e2e163
Update GbtsSeedingAlgorithm.cpp
Rosie-Hasan Jan 14, 2025
f8edb15
Update SeedFinderGbtsConfig.hpp
Rosie-Hasan Jan 14, 2025
e1709e9
Update SeedFinderGbtsConfig.hpp
Rosie-Hasan Jan 14, 2025
05e3af2
Update SeedFinderGbtsConfig.hpp
Rosie-Hasan Jan 14, 2025
ae8cd35
adding documentation for GBTS space point IDs
Rosie-Hasan Jan 15, 2025
b5a1ded
adding check for empty source link recommended my code rabbit
Rosie-Hasan Jan 15, 2025
83c742c
forgot to run clang format on changes
Rosie-Hasan Jan 15, 2025
831f82b
fixing mistake I made taking code rabbit suggestion
Rosie-Hasan Jan 15, 2025
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
38 changes: 22 additions & 16 deletions Core/include/Acts/Seeding/GbtsDataStorage.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,28 @@ constexpr std::size_t N_SEG_CONNS = 6; // was 6
template <typename space_point_t>
struct GbtsSP {
const space_point_t *SP; // want inside to have pointer
int gbtsID;
int combined_ID;
GbtsSP(const space_point_t *sp, int id, int combined_id)
: SP(sp), gbtsID(id), combined_ID{combined_id} {
if (SP->sourceLinks().size() == 1) { // pixels have 1 SL
m_isPixel = true;
} else {
m_isPixel = false;
}
m_phi = std::atan(SP->x() / SP->y());
int gbtsID; // used to access detector layer information in connector input
// file
int combined_ID; // includes eta id which references the detector module
bool m_isPixel;
float m_phi;
float m_r;
float m_ClusterWidth;
GbtsSP(const space_point_t *sp, int id, int combined_id, bool isPixel,
float ClusterWidth)
: SP(sp),
gbtsID(id),
combined_ID{combined_id},
m_isPixel(isPixel),
m_ClusterWidth(ClusterWidth) {
m_phi = std::atan2(SP->y(), SP->x());
m_r = std::sqrt((SP->x() * SP->x()) + (SP->y() * SP->y()));
};
bool isPixel() const { return m_isPixel; }
bool isSCT() const { return !m_isPixel; }
bool isStrip() const { return !m_isPixel; }
Comment on lines 47 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a lot detector dependent. Not for now, but probably we need to define a strategy to generalise this code.

float phi() const { return m_phi; }
bool m_isPixel;
float m_phi;
float r() const { return m_r; }
float ClusterWidth() const { return m_ClusterWidth; }
};

template <typename space_point_t>
Expand Down Expand Up @@ -152,7 +158,7 @@ class GbtsDataStorage {
return -1;
}

int binIndex = pL->getEtaBin(sp.SP->z(), sp.SP->r());
int binIndex = pL->getEtaBin(sp.SP->z(), sp.r());
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Error codes, document we must. Return values, explain we should.

Return values (-1, -2, -3), their meaning unclear it is. Documentation for error conditions, add you must.

Add enum for error codes:

+  enum class SpacePointError {
+    Success = 0,
+    InvalidLayer = -1,
+    InvalidEtaBin = -2,
+    InvalidClusterWidth = -3
+  };

Also applies to: 185-189


if (binIndex == -1) {
return -2;
Expand All @@ -165,7 +171,7 @@ class GbtsDataStorage {
float max_tau = 100.0;
// can't do this bit yet as dont have cluster width
if (useClusterWidth) {
float cluster_width = 1; // temporary while cluster width not available
float cluster_width = sp.ClusterWidth();
min_tau = 6.7 * (cluster_width - 0.2);
max_tau =
1.6 + 0.15 / (cluster_width + 0.2) + 6.1 * (cluster_width - 0.2);
Comment on lines +174 to 177
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Magic numbers in tau calculations, explain you must.

Constants 6.7, 0.2, 1.6, 0.15, and 6.1, mysterious they are. Document their significance and origin, you should.

Extract as named constants with documentation:

+  // Tau calculation constants for barrel layers
+  static constexpr float MIN_TAU_FACTOR = 6.7f;
+  static constexpr float CLUSTER_WIDTH_OFFSET = 0.2f;
+  static constexpr float MAX_TAU_BASE = 1.6f;
+  static constexpr float MAX_TAU_FACTOR1 = 0.15f;
+  static constexpr float MAX_TAU_FACTOR2 = 6.1f;

Committable suggestion skipped: line range outside the PR's diff.

Expand All @@ -176,7 +182,7 @@ class GbtsDataStorage {
sp, min_tau, max_tau)); // adding ftf member to nodes
} else {
if (useClusterWidth) {
float cluster_width = 1; // temporary while cluster width not available
float cluster_width = sp.ClusterWidth();
if (cluster_width > 0.2) {
return -3;
}
Expand Down
6 changes: 3 additions & 3 deletions Core/include/Acts/Seeding/GbtsTrackingFilter.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ struct GbtsEdgeState {
// x' = x*m_c + y*m_s
// y' = -x*m_s + y*m_c

m_refY = pS->m_n2->m_spGbts.SP->r();
m_refY = pS->m_n2->m_spGbts.r();
m_refX =
pS->m_n2->m_spGbts.SP->x() * m_c + pS->m_n2->m_spGbts.SP->y() * m_s;

Expand All @@ -67,7 +67,7 @@ struct GbtsEdgeState {

m_Y[0] = pS->m_n2->m_spGbts.SP->z();
m_Y[1] = (pS->m_n1->m_spGbts.SP->z() - pS->m_n2->m_spGbts.SP->z()) /
(pS->m_n1->m_spGbts.SP->r() - pS->m_n2->m_spGbts.SP->r());
(pS->m_n1->m_spGbts.r() - pS->m_n2->m_spGbts.r());

memset(&m_Cx[0][0], 0, sizeof(m_Cx));
memset(&m_Cy[0][0], 0, sizeof(m_Cy));
Expand Down Expand Up @@ -276,7 +276,7 @@ class GbtsTrackingFilter {
x = pS->m_n1->m_spGbts.SP->x();
y = pS->m_n1->m_spGbts.SP->y();
z = pS->m_n1->m_spGbts.SP->z();
r = pS->m_n1->m_spGbts.SP->r();
r = pS->m_n1->m_spGbts.r();

refX = x * ts.m_c + y * ts.m_s;
mx = -x * ts.m_s + y * ts.m_c; // measured X[0]
Expand Down
13 changes: 6 additions & 7 deletions Core/include/Acts/Seeding/SeedFinderGbts.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
// TODO: update to C++17 style

#include "Acts/Geometry/Extent.hpp"
#include "Acts/Seeding/SeedFilter.hpp"
#include "Acts/Seeding/SeedFinder.hpp"
#include "Acts/Seeding/SeedFinderGbtsConfig.hpp"
#include "Acts/Seeding/SeedFinderUtils.hpp"
Expand Down Expand Up @@ -166,11 +165,11 @@ void SeedFinderGbts<external_spacepoint_t>::runGbts_TrackFinder(
continue;
}

float r1 = n1->m_spGbts.SP->r();
float r1 = n1->m_spGbts.r();
float x1 = n1->m_spGbts.SP->x();
float y1 = n1->m_spGbts.SP->y();
float z1 = n1->m_spGbts.SP->z();
float phi1 = std::atan(x1 / y1);
float phi1 = n1->m_spGbts.phi();

float minPhi = phi1 - deltaPhi;
float maxPhi = phi1 + deltaPhi;
Expand Down Expand Up @@ -199,7 +198,7 @@ void SeedFinderGbts<external_spacepoint_t>::runGbts_TrackFinder(
continue;
}

float r2 = n2->m_spGbts.SP->r();
float r2 = n2->m_spGbts.r();

float dr = r2 - r1;

Expand Down Expand Up @@ -545,7 +544,7 @@ void SeedFinderGbts<external_spacepoint_t>::runGbts_TrackFinder(

for (unsigned int idx_m = 1; idx_m < vSP.size() - 1; idx_m++) {
const GbtsSP<external_spacepoint_t>& spM = *vSP.at(idx_m);
const double pS_r = spM.SP->r();
const double pS_r = spM.r();
const double pS_x = spM.SP->x();
const double pS_y = spM.SP->y();
const double cosA = pS_x / pS_r;
Expand Down Expand Up @@ -676,8 +675,8 @@ void SeedFinderGbts<external_spacepoint_t>::createSeeds(
} else {
// In normal (non LRT) mode penalise SSS by 1000, PSS (if enabled) and
// PPS by 10000
Comment on lines 676 to 677
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can neglect the LRT case for the moment. Also, I do not think we build PSS and/or PPS at all, so we can probably remove those cases?

if (seed.s3().isSCT()) {
newQ += seed.s1().isSCT() ? 1000.0 : 10000.0;
if (seed.s3().isStrip()) {
newQ += seed.s1().isStrip() ? 1000.0 : 10000.0;
}
}
seed.Q(newQ);
Expand Down
10 changes: 2 additions & 8 deletions Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@
// core algorithm so in acts namespace
namespace Acts {

template <typename T>
class SeedFilter;

template <typename SpacePoint>
struct SeedFinderGbtsConfig {
// // how many sigmas of scattering angle should be considered?
Expand All @@ -31,16 +28,12 @@ struct SeedFinderGbtsConfig {
// Seed cut
float minPt = 400. * Acts::UnitConstants::MeV;

///////////some declared not filled in by reco: //////
std::shared_ptr<Acts::SeedFilter<SpacePoint>> seedFilter;

// //detector ROI
// // derived values, set on SeedFinder construction
float highland = 0;
float maxScatteringAngle2 = 0;
// bool isInInternalUnits = false;
/// for load space points
unsigned int maxSeedsPerSpM = 5;

// Parameter which can loosen the tolerance of the track seed to form a
// helix. This is useful for e.g. misaligned seeding.
Expand All @@ -50,7 +43,8 @@ struct SeedFinderGbtsConfig {
float m_nMaxPhiSlice = 53; // used to calculate phi slices
bool m_useClusterWidth =
false; // bool for use of cluster width in loadSpacePoints function
std::string connector_input_file; // input file for connector object
std::string ConnectorInputFile; // Path to the connector configuration file
// that defines the layer connections
std::vector<TrigInDetSiLayer> m_layerGeometry;

// for runGbts_TrackFinder
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

#pragma once

#include "Acts/Seeding/SeedFilterConfig.hpp"
#include "Acts/Seeding/SeedFinderGbts.hpp"
#include "Acts/Seeding/SeedFinderGbtsConfig.hpp"
#include "ActsExamples/EventData/Cluster.hpp"
Expand All @@ -36,7 +35,6 @@ class GbtsSeedingAlgorithm final : public IAlgorithm {

std::string outputSeeds;

Acts::SeedFilterConfig seedFilterConfig;
Acts::SeedFinderGbtsConfig<SimSpacePoint> seedFinderConfig;
Acts::SeedFinderOptions seedFinderOptions;

Expand Down
30 changes: 16 additions & 14 deletions Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include "ActsExamples/TrackFinding/GbtsSeedingAlgorithm.hpp"

#include "Acts/Geometry/GeometryIdentifier.hpp"
#include "Acts/Seeding/SeedFilter.hpp"
#include "ActsExamples/EventData/IndexSourceLink.hpp"
#include "ActsExamples/EventData/Measurement.hpp"
#include "ActsExamples/EventData/ProtoTrack.hpp"
Expand Down Expand Up @@ -39,8 +38,6 @@ ActsExamples::GbtsSeedingAlgorithm::GbtsSeedingAlgorithm(
// fill config struct
m_cfg.layerMappingFile = m_cfg.layerMappingFile;

m_cfg.seedFilterConfig = m_cfg.seedFilterConfig.toInternalUnits();

m_cfg.seedFinderConfig =
m_cfg.seedFinderConfig.toInternalUnits().calculateDerivedQuantities();

Expand Down Expand Up @@ -70,7 +67,7 @@ ActsExamples::GbtsSeedingAlgorithm::GbtsSeedingAlgorithm(
m_cfg.seedFinderConfig.m_layerGeometry = LayerNumbering();

std::ifstream input_ifstream(
m_cfg.seedFinderConfig.connector_input_file.c_str(), std::ifstream::in);
m_cfg.seedFinderConfig.ConnectorInputFile.c_str(), std::ifstream::in);

// connector
std::unique_ptr<Acts::GbtsConnector> inputConnector =
Expand All @@ -89,16 +86,14 @@ ActsExamples::ProcessCode ActsExamples::GbtsSeedingAlgorithm::execute(
MakeGbtsSpacePoints(ctx, m_cfg.ActsGbtsMap);

for (auto sp : GbtsSpacePoints) {
ACTS_DEBUG("Gbts space points: Gbts_id: "
<< sp.gbtsID << " z: " << sp.SP->z() << " r: " << sp.SP->r()
<< " ACTS volume: "
<< sp.SP->sourceLinks()
.front()
.get<IndexSourceLink>()
.geometryId()
.volume());
const auto &links = sp.SP->sourceLinks();
if (!links.empty()) {
ACTS_DEBUG("Gbts space points: Gbts_id: "
<< sp.gbtsID << " z: " << sp.SP->z() << " r: " << sp.SP->r()
<< " ACTS volume: "
<< links.front().get<IndexSourceLink>().geometryId().volume());
}
}

// this is now calling on a core algorithm
Acts::SeedFinderGbts<SimSpacePoint> finder(
m_cfg.seedFinderConfig, *m_gbtsGeo,
Expand Down Expand Up @@ -222,8 +217,15 @@ ActsExamples::GbtsSeedingAlgorithm::MakeGbtsSpacePoints(
int eta_mod = Find->second.second;
int combined_id = Gbts_id * 1000 + eta_mod;

// check if SP is pixel, dependent of type of SP so must be done in
// examples
bool isPixel = (sourceLink.size() == 1); // pixels have 1 SL

float ClusterWidth =
0; // false input as this is not available in examples
// fill Gbts vector with current sapce point and ID
gbtsSpacePoints.emplace_back(&spacePoint, Gbts_id, combined_id);
gbtsSpacePoints.emplace_back(&spacePoint, Gbts_id, combined_id, isPixel,
ClusterWidth); // make new GbtsSP here !
}
}
ACTS_VERBOSE("Space points successfully assigned Gbts ID");
Expand Down
35 changes: 5 additions & 30 deletions Examples/Python/python/acts/examples/reconstruction.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ def addSeeding(
field: acts.MagneticFieldProvider,
geoSelectionConfigFile: Optional[Union[Path, str]] = None,
layerMappingConfigFile: Optional[Union[Path, str]] = None,
connector_inputConfigFile: Optional[Union[Path, str]] = None,
ConnectorInputConfigFile: Optional[Union[Path, str]] = None,
seedingAlgorithm: SeedingAlgorithm = SeedingAlgorithm.Default,
trackSmearingSigmas: TrackSmearingSigmas = TrackSmearingSigmas(),
initialSigmas: Optional[list] = None,
Expand Down Expand Up @@ -430,12 +430,11 @@ def addSeeding(
spacePoints,
seedFinderConfigArg,
seedFinderOptionsArg,
seedFilterConfigArg,
trackingGeometry,
logLevel,
layerMappingConfigFile,
geoSelectionConfigFile,
connector_inputConfigFile,
ConnectorInputConfigFile,
)
elif seedingAlgorithm == SeedingAlgorithm.Hashing:
logger.info("Using Hashing seeding")
Expand Down Expand Up @@ -1088,24 +1087,22 @@ def addGbtsSeeding(
spacePoints: str,
seedFinderConfigArg: SeedFinderConfigArg,
seedFinderOptionsArg: SeedFinderOptionsArg,
seedFilterConfigArg: SeedFilterConfigArg,
trackingGeometry: acts.TrackingGeometry,
logLevel: acts.logging.Level = None,
layerMappingConfigFile: Union[Path, str] = None,
geoSelectionConfigFile: Union[Path, str] = None,
connector_inputConfigFile: Union[Path, str] = None,
ConnectorInputConfigFile: Union[Path, str] = None,
):
"""Gbts seeding"""

logLevel = acts.examples.defaultLogging(sequence, logLevel)()
layerMappingFile = str(layerMappingConfigFile) # turn path into string
connector_inputFile = str(connector_inputConfigFile)
ConnectorInputFileStr = str(ConnectorInputConfigFile)
seedFinderConfig = acts.SeedFinderGbtsConfig(
**acts.examples.defaultKWArgs(
sigmaScattering=seedFinderConfigArg.sigmaScattering,
maxSeedsPerSpM=seedFinderConfigArg.maxSeedsPerSpM,
minPt=seedFinderConfigArg.minPt,
connector_input_file=connector_inputFile,
ConnectorInputFile=ConnectorInputFileStr,
m_useClusterWidth=False,
),
)
Expand All @@ -1121,33 +1118,11 @@ def addGbtsSeeding(
bFieldInZ=seedFinderOptionsArg.bFieldInZ,
)
)
seedFilterConfig = acts.SeedFilterConfig(
**acts.examples.defaultKWArgs(
maxSeedsPerSpM=seedFinderConfig.maxSeedsPerSpM,
deltaRMin=(
seedFinderConfigArg.deltaR[0]
if seedFilterConfigArg.deltaRMin is None
else seedFilterConfigArg.deltaRMin
),
impactWeightFactor=seedFilterConfigArg.impactWeightFactor,
zOriginWeightFactor=seedFilterConfigArg.zOriginWeightFactor,
compatSeedWeight=seedFilterConfigArg.compatSeedWeight,
compatSeedLimit=seedFilterConfigArg.compatSeedLimit,
numSeedIncrement=seedFilterConfigArg.numSeedIncrement,
seedWeightIncrement=seedFilterConfigArg.seedWeightIncrement,
seedConfirmation=seedFilterConfigArg.seedConfirmation,
# curvatureSortingInFilter=seedFilterConfigArg.curvatureSortingInFilter,
maxSeedsPerSpMConf=seedFilterConfigArg.maxSeedsPerSpMConf,
maxQualitySeedsPerSpMConf=seedFilterConfigArg.maxQualitySeedsPerSpMConf,
useDeltaRorTopRadius=seedFilterConfigArg.useDeltaRorTopRadius,
)
)

seedingAlg = acts.examples.GbtsSeedingAlgorithm(
level=logLevel,
inputSpacePoints=[spacePoints],
outputSeeds="seeds",
seedFilterConfig=seedFilterConfig,
seedFinderConfig=seedFinderConfig,
seedFinderOptions=seedFinderOptions,
layerMappingFile=layerMappingFile,
Expand Down
9 changes: 4 additions & 5 deletions Examples/Python/src/TrackFinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,11 @@ void addTrackFinding(Context& ctx) {
ACTS_PYTHON_MEMBER(sigmaScattering);
ACTS_PYTHON_MEMBER(highland);
ACTS_PYTHON_MEMBER(maxScatteringAngle2);
ACTS_PYTHON_MEMBER(connector_input_file);
ACTS_PYTHON_MEMBER(ConnectorInputFile);
ACTS_PYTHON_MEMBER(m_phiSliceWidth);
ACTS_PYTHON_MEMBER(m_nMaxPhiSlice);
ACTS_PYTHON_MEMBER(m_useClusterWidth);
ACTS_PYTHON_MEMBER(m_layerGeometry);
ACTS_PYTHON_MEMBER(maxSeedsPerSpM);
ACTS_PYTHON_STRUCT_END();
patchKwargsConstructor(c);
}
Expand Down Expand Up @@ -273,9 +272,9 @@ void addTrackFinding(Context& ctx) {

ACTS_PYTHON_DECLARE_ALGORITHM(
ActsExamples::GbtsSeedingAlgorithm, mex, "GbtsSeedingAlgorithm",
inputSpacePoints, outputSeeds, seedFilterConfig, seedFinderConfig,
seedFinderOptions, layerMappingFile, geometrySelection, trackingGeometry,
ActsGbtsMap, fill_module_csv, inputClusters);
inputSpacePoints, outputSeeds, seedFinderConfig, seedFinderOptions,
layerMappingFile, geometrySelection, trackingGeometry, ActsGbtsMap,
fill_module_csv, inputClusters);

ACTS_PYTHON_DECLARE_ALGORITHM(
ActsExamples::HoughTransformSeeder, mex, "HoughTransformSeeder",
Expand Down
2 changes: 1 addition & 1 deletion Examples/Scripts/Python/full_chain_itk_Gbts.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
),
geoSelectionConfigFile=geo_dir / "itk-hgtd/geoSelection-ITk.json",
layerMappingConfigFile=geo_dir / "itk-hgtd/ACTS_FTF_mapinput.csv",
connector_inputConfigFile=geo_dir / "itk-hgtd/binTables_ITK_RUN4.txt",
ConnectorInputConfigFile=geo_dir / "itk-hgtd/binTables_ITK_RUN4.txt",
outputDirRoot=outputDir,
)

Expand Down
Loading