Skip to content

Common: Adding a protection in the collisionAssociator task#3592

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

Merged
jgrosseo merged 3 commits into from
Oct 16, 2023
Merged

Common: Adding a protection in the collisionAssociator task #3592

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0514e58
PWGHF: Adding protection for tracks associated with a 0-dimensional B…
Oct 11, 2023
99453d0
Please consider the following formatting changes
alibuild Oct 11, 2023
3d71226
Merge pull request #3 from alibuild/alibot-cleanup-3592
DelloStritto Oct 11, 2023
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
7 changes: 7 additions & 0 deletions Common/Core/CollisionAssociation.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -133,6 +133,10 @@ class CollisionAssociation
for (const auto& ambTrack : ambiguousTracks) {
if constexpr (isCentralBarrel) { // FIXME: to be removed as soon as it is possible to use getId<Table>() for joined tables
if (ambTrack.trackId() == track.globalIndex()) {
if (!ambTrack.has_bc() || ambTrack.bc().size() == 0) {

@aalkinaalkinOct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is redundant, for the slice index has_ method checks if both start and end indices are non-negative. The size of the valid slice can not be less than 1.

@fgrosafgrosaOct 11, 2023•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @aalkin thanks for the feedback! We checked with @DelloStritto one file where the previous code was crashing, and the ambTrack.has_bc() condition was not sufficient, we had to add the second one.

@DelloStrittoDelloStrittoOct 11, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hi Anton,
with the condition !ambTrack.has_bc() we did not manage to reject the track which was making the task crash while we did it with the requirement ambTrack.bc().size() == 0, so I am not sure they are fully equivalent.

Do you suggest to remove the condition !ambTrack.has_bc()?

Thanks a lot for the review,
Luigi

@aalkinaalkinOct 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The getter for the slice index looks like this

template <typename T>
auto _Getter_##_as() const
{
if (O2_BUILTIN_UNLIKELY(mBinding == nullptr)) {
o2::soa::notBoundTable(#_Table_);
}
if (O2_BUILTIN_UNLIKELY(!has_##_Getter_())) {
returnstatic_cast<T const*>(mBinding)->emptySlice();
}
auto a = *mColumnIterator;
auto t = static_cast<T const*>(mBinding)->rawSlice(a[0], a[1]);
static_cast<T const*>(mBinding)->copyIndexBindings(t);
return t;
}

Size = 0 can only happen if has_ condition is already false, or the data is somehow malformed. Probably there is an entry where start > end?

@jgrosseojgrosseoOct 13, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a[0] cannot be less than a[1]?

@aalkinaalkinOct 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is unexpected, the slice with start=end should be size 1. I'll try to reproduce this with the test.

@fgrosafgrosaOct 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@DelloStritto@aalkin thanks for the checks! @jgrosseo can we in the meantime merge this PR? Based on the last messages, I would say that we should also keep the check on the size to properly catch the pathological tracks in the existing MC productions. Thanks!

@jgrosseojgrosseoOct 16, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not against merging this. @aalkin Let us know when you have news...

@aalkinaalkinOct 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've added the test for start==end slice index (AliceO2Group/AliceO2#12088), and the slice of a correct size of 1 is returned. Let me look at the problematic data file, because the issue you've found seems to be something else.

@DelloStrittoDelloStrittoOct 16, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ok, I can share the file I am testing with you, thanks @aalkin

globalBC.push_back(-1);
break;
}
globalBC.push_back(ambTrack.bc().begin().globalBC());
break;
}
Expand DownExpand Up@@ -163,6 +167,9 @@ class CollisionAssociation
}

float trackTime = track.trackTime();
if (globalBC[track.filteredIndex()] < 0) {
continue;
}
const int64_t bcOffsetWindow = (int64_t)globalBC[track.filteredIndex()] + trackTime / o2::constants::lhc::LHCBunchSpacingNS - (int64_t)collBC;
if (std::abs(bcOffsetWindow) > bOffsetMax) {
continue;
Expand Down