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

fix: A bug in the Triplet finding algorithm #1025

Merged

Conversation

konradkusiak97
Copy link

As we found out with @krasznaa, there was a bug in the current implementation of the Sycl Seedfinder algorithm. It came out in a particular case when the first part of the algorithm - the Duplet finding, didn't find any duplets. In this scenario, the Triplet finding was running forever and the code didn't terminate.

The problem appeared inside the loop of the Triplet finding.

In the current implementation, finding triplets is splitted into several parts to limit the memory allocations on the GPU. The range of the particular iteration is taken from the firstMiddle and lastMiddle indices. The current logic follows somewhat those steps:

firstMiddle = lastMiddle = 0
while  firstMiddle < MiddleSPs 
   do 
     ++lastMiddle                  // until maxMemory is reached
     numThreads = calcThreads()    // calculate the number of threads for the GPU kernel
     if numThreads != 0:
        tripletSearch(numThreads)
    else: 
        continue
    firstMiddle = lastMiddle

Currently, the increment of the lastMiddle index is performed under the condition. This condition is not satisfied if there are 0 Duplets found and therefore both firstMiddle and lastMiddle stay always 0.

The fix for this issue is to increment the lastMiddle index even when there are 0 numThreads calculated and then continue to the next iteration.

@krasznaa krasznaa added this to the next milestone Oct 5, 2021
@krasznaa
Copy link
Member

krasznaa commented Oct 5, 2021

@paulgessinger, why is the CI not starting on Konrad's PR? It was not starting on his previous PR either, until I started pusing some of my own commits to it. 😕

@paulgessinger
Copy link
Member

@krasznaa weird. I think it should, since @konradkusiak97 is not a first-time contributor anymore, so that should be not the problem. Very strange.

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #1025 (4dc8095) into main (6f3abdd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1025   +/-   ##
=======================================
  Coverage   48.62%   48.62%           
=======================================
  Files         337      337           
  Lines       17334    17334           
  Branches     8192     8192           
=======================================
  Hits         8428     8428           
  Misses       3164     3164           
  Partials     5742     5742           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f3abdd...4dc8095. Read the comment docs.

@robertlangenberg
Copy link
Contributor

@krasznaa CI ran through now, if this change looks good, please approve and this can go in

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Ohh, yes. I'm fully on board with this. 👍

@robertlangenberg robertlangenberg merged commit c607476 into acts-project:main Oct 6, 2021
@paulgessinger paulgessinger modified the milestones: next, v14.0.0 Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants