-
Notifications
You must be signed in to change notification settings - Fork 17
Implement substructure search with full recursion #81
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
base: main
Are you sure you want to change the base?
Conversation
Greptile Overview
|
| Filename | Overview |
|---|---|
| src/substruct/substruct_search.cu | Main pipelined substructure search implementation with multi-threading and GPU execution orchestration |
| src/substruct/substruct_kernels.cu | CUDA kernels for label matrix computation and substructure matching with GSI algorithm |
| src/substruct/recursive_preprocessor.cu | Recursive SMARTS pattern preprocessing with depth-based batching and double-buffering |
| src/substruct/recursive_preprocessor.h | Header for recursive SMARTS preprocessing with LeafSubpatterns and RecursiveScratchBuffers |
| src/substruct/minibatch_planner.cpp | CPU-side mini-batch planning and pipeline scheduling for recursive patterns |
| tests/test_substruct_search.cu | Comprehensive test suite with 1935 lines covering various substructure search scenarios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 files reviewed, no comments
| constexpr int kOverflowEntriesPerBuffer = 2048; | ||
|
|
||
| /// Maximum nesting depth for recursive SMARTS patterns | ||
| constexpr int kMaxSmartsNestingDepth = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, does RDKit also assume a maximum nesting depth of 4? If not, what happens if the nesting depth exceeds 4 in our implementation? Fall back to calling RDKit?
|
|
||
| } else if constexpr (Algo == SubstructAlgorithm::GSI) { | ||
| constexpr int kBlockSizeT = getBlockSizeForConfig<MaxTargetAtoms>(); | ||
| constexpr int kMaxPartialsT = getMaxPartialsForSM<MaxTargetAtoms, MaxQueryAtoms>(86, kBlockSizeT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be trivial, but why we choose to hard-code 86?
| std::vector<std::vector<std::vector<int>>*> matchRefs; | ||
| matchRefs.reserve(updates.size()); | ||
|
|
||
| { | ||
| std::lock_guard<std::mutex> lock(resultsMutex); | ||
| for (const auto& u : updates) { | ||
| matchRefs.push_back(&results.getMatchesMut(u.targetIdx, u.queryIdx)); | ||
| } | ||
| } | ||
|
|
||
| for (size_t i = 0; i < updates.size(); ++i) { | ||
| const auto& u = updates[i]; | ||
| auto& targetMatches = *matchRefs[i]; | ||
|
|
||
| targetMatches.reserve(targetMatches.size() + u.reportedMatches); | ||
| const int16_t* src = hostBuffer.matchIndices.data() + u.miniBatchLocalOffset; | ||
|
|
||
| for (int m = 0; m < u.reportedMatches; ++m) { | ||
| auto& match = targetMatches.emplace_back(u.queryAtoms); | ||
| for (int a = 0; a < u.queryAtoms; ++a) { | ||
| match[a] = src[m * u.queryAtoms + a]; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, here the creation of matchRefs is inside the lock, but the writes to targetMatches are outside the lock. Will there be a race condition concern?"
| // Instantiate parameterized tests for all algorithms | ||
| INSTANTIATE_TEST_SUITE_P(AllAlgorithms, | ||
| SubstructureSearchTest, | ||
| ::testing::Values(SubstructAlgorithm::GSI), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double confirm here if we only want to test GSI but not VF2. I know VF2 is mostly used internally as reference, but the use of AllAlgorithms here can be a little confusing.
evasnow1992
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for you putting all these together. A few clarification questions.
No description provided.