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.

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

[14_0_X] Removed variable-length arrays #44936

Merged
merged 1 commit into from
May 14, 2024
Merged

[14_0_X] Removed variable-length arrays #44936

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 9 additions & 17 deletions L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,19 +184,11 @@ void PurgeDuplicate::execute(std::vector<Track>& outputtracks, unsigned int iSec
}
}

// Initialize all-false 2D array of tracks being duplicates to other tracks
bool dupMap[numStublists][numStublists]; // Ends up symmetric
for (unsigned int itrk = 0; itrk < numStublists; itrk++) {
for (unsigned int jtrk = 0; jtrk < numStublists; jtrk++) {
dupMap[itrk][jtrk] = false;
}
}
// Initialize all-false 2D vector of tracks being duplicates to other tracks
vector<vector<bool>> dupMap(numStublists, vector<bool>(numStublists, false));
Copy link
Contributor

Choose a reason for hiding this comment

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

If symmetric and storage is an issue why not using triangular storage?
btw interesting enough vector<bool> uses one bit per entry while bool v[N] will use a full byte...


// Used to check if a track is in two bins, is not a duplicate in either bin, so is sent out twice
bool noMerge[numStublists];
for (unsigned int itrk = 0; itrk < numStublists; itrk++) {
noMerge[itrk] = false;
}
vector<bool> noMerge(numStublists, false);

// Find duplicates; Fill dupMap by looping over all pairs of "tracks"
// numStublists-1 since last track has no other to compare to
Expand Down Expand Up @@ -286,24 +278,24 @@ void PurgeDuplicate::execute(std::vector<Track>& outputtracks, unsigned int iSec

// Fill duplicate map
if (nShareLay >= settings_.minIndStubs()) { // For number of shared stub merge condition
dupMap[itrk][jtrk] = true;
dupMap[jtrk][itrk] = true;
dupMap.at(itrk).at(jtrk) = true;
dupMap.at(jtrk).at(itrk) = true;
}
}
}

// Check to see if the track is a duplicate
for (unsigned int itrk = 0; itrk < numStublists; itrk++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why this loop at all?
just set noMerge[itrk] = true; where one set the dupMap.

for (unsigned int jtrk = 0; jtrk < numStublists; jtrk++) {
if (dupMap[itrk][jtrk]) {
noMerge[itrk] = true;
if (dupMap.at(itrk).at(jtrk)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why at?
please use at only for debugging, never in production code.

noMerge.at(itrk) = true;
}
}
}

// If the track isn't a duplicate, and if it's in more than one bin, and it is not in the proper rinv or phi bin, then mark it so it won't be sent to output
for (unsigned int itrk = 0; itrk < numStublists; itrk++) {
if (noMerge[itrk] == false) {
if (!noMerge.at(itrk)) {
if (((findOverlapRinvBins(inputtracklets_[itrk]).size() > 1) &&
(findRinvBin(inputtracklets_[itrk]) != bin)) ||
((findOverlapPhiBins(inputtracklets_[itrk]).size() > 1) &&
Expand All @@ -316,7 +308,7 @@ void PurgeDuplicate::execute(std::vector<Track>& outputtracks, unsigned int iSec
for (unsigned int itrk = 0; itrk < numStublists - 1; itrk++) {
for (unsigned int jtrk = itrk + 1; jtrk < numStublists; jtrk++) {
// Merge a track with its first duplicate found.
Copy link
Contributor

@VinInn VinInn May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

here is said "merge with the first duplicate found" but I no not see any break, so what prevents to merge with all other duplicates?

If really one merges with just the first found than there is no need of the dupMap: it is enough for each track to keep record of the first duplicate...

I think the whole algorithm need a review as most probably all those large maps and vectors are not needed...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

if (dupMap[itrk][jtrk]) {
if (dupMap.at(itrk).at(jtrk)) {
// Set preferred track based on seed rank
int preftrk;
int rejetrk;
Expand Down