Skip to content

MCS Fast match pathway - #238

Open
scal444 wants to merge 4 commits into
NVIDIA-BioNeMo:mainfrom
scal444:mcs-split-03-fast-match
Open

MCS Fast match pathway#238
scal444 wants to merge 4 commits into
NVIDIA-BioNeMo:mainfrom
scal444:mcs-split-03-fast-match

Conversation

@scal444

@scal444 scal444 commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Attempts a quick match of a seed before falling back to (not yet implemented) full substructure search.

scal444 added 3 commits July 28, 2026 12:52
There is exactly one topology representation. The QueryTopology /
TargetTopology template parameters and the kHasAdjacencyBondIndices
trait made it look like there were two, but the only type that ever set
the trait false was a test double -- production code has always passed a
CSR view with all four arrays populated.

The trait dates from when the CSR-adjacency scan was added on top of the
original linear bond-endpoint scan and the old path was kept alive
behind `if constexpr`. Both paths now land together, so the fallback was
dead on arrival: each `else` branch held a verbatim copy of the
adjacency loop under a runtime null-check plus a linear scan nothing
reaches.

Hoist DeviceCsrView into a new fmcs_topology.cuh (along with the bond
endpoint pack/unpack constants, which fmcs_grow.cuh had been
duplicating), take it concretely in matchSingleBondWithinThread,
matchIncrementalFastCooperative, and fillNewBondsCooperative, and delete
the trait with both fallback branches. fmcs_match.cuh drops from 453 to
321 lines with no behaviour change.

The match tests relied on the fallback -- they never populated CSR at
all -- so they now build real CSR arrays via a TestGraph helper and
exercise the path that actually ships.
@scal444
scal444 requested a review from evasnow1992 July 28, 2026 17:26
Comment thread src/mcs/fmcs_cuda/fmcs_match.cuh
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces an FMCS CUDA fast-match pathway and supporting topology representation.

  • Adds single-bond compatibility matching and cooperative greedy incremental matching.
  • Centralizes packed bond-endpoint constants and adds a non-owning CSR topology view.
  • Updates grow helpers to consume the topology view and documents the required exact fallback.
  • Adds CUDA unit tests and registers the new test target.

Confidence Score: 3/5

The PR does not yet appear safe to merge because the previously reported valid-embedding loss has not been shown to be eliminated by an exact fallback.

The greedy matcher still commits mappings without backtracking and can return an inconclusive false result after those mutations; although its contract now requires an exact fallback, the current code does not establish production integration that prevents this result from pruning a valid growth path.

Files Needing Attention: src/mcs/fmcs_cuda/fmcs_match.cuh

Important Files Changed

Filename Overview
src/mcs/fmcs_cuda/fmcs_match.cuh Adds single-bond matching, incremental cooperative matching, and fallback scratch storage.
src/mcs/fmcs_cuda/fmcs_grow.cuh Switches growth topology handling to DeviceCsrView and documents callback fallback responsibilities.
src/mcs/fmcs_cuda/fmcs_topology.cuh Defines shared bond-endpoint encoding constants and the device CSR topology view.
tests/test_fmcs_match.cu Adds CUDA unit coverage for single-bond and incremental greedy matching behavior.
tests/test_fmcs_grow.cu Updates growth tests to construct the new DeviceCsrView type.
tests/CMakeLists.txt Registers and links the new FMCS matching test executable.

Reviews (2): Last reviewed commit: "Clarify greedy fMCS match contract" | Re-trigger Greptile

@scal444

scal444 commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Towards #221

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.

1 participant