Skip to content

Uff python bindings#129

Open
scal444 wants to merge 3 commits intoNVIDIA-Digital-Bio:mainfrom
scal444:uff_python
Open

Uff python bindings#129
scal444 wants to merge 3 commits intoNVIDIA-Digital-Bio:mainfrom
scal444:uff_python

Conversation

@scal444
Copy link
Copy Markdown
Collaborator

@scal444 scal444 commented Apr 7, 2026

No description provided.

@scal444 scal444 requested a review from evasnow1992 April 7, 2026 16:51
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 7, 2026

Greptile Summary

Adds GPU-accelerated UFF force-field optimization as a new Python binding (UFFOptimizeMoleculesConfs), following the established MMFF pattern. Common BFGS helpers are extracted into bfgs_common.cpp/h and shared between the MMFF and UFF backends.

Confidence Score: 5/5

Safe to merge; all findings are P2 style and performance suggestions with no correctness impact.

The implementation closely mirrors the existing MMFF pattern. Three P2 findings: weak stub types, an unchecked embedding return in a test helper, and a missed FF-params caching opportunity. None affect runtime correctness or data integrity.

No files require special attention for merge safety.

Important Files Changed

Filename Overview
src/minimizer/bfgs_uff.cpp New UFF BFGS minimizer; missing per-molecule FF-param caching present in the MMFF counterpart
nvmolkit/uffOptimization.py New Python wrapper for UFF optimization; consistent scalar-or-sequence normalisation matching MMFF pattern
nvmolkit/uffOptimization.cpp New Boost.Python bindings; clean extraction helpers with null-pointer guard
nvmolkit/tests/test_uff_optimization.py New UFF test suite; make_fragmented_mol does not check EmbedMultipleConfs return value
nvmolkit/_uffOptimization.pyi New C++ binding stub; vdwThresholds and ignoreInterfragInteractions typed as Any
src/minimizer/bfgs_common.cpp Shared BFGS helpers extracted from bfgs_mmff; energies buffer sizing corrected vs. original
src/minimizer/bfgs_common.h New shared header for ThreadLocalBuffers, ConformerInfo, and BatchExecutionContext
src/minimizer/bfgs_uff.h New UFF minimizer header; clean signature in dedicated UFF namespace
src/minimizer/bfgs_mmff.cpp Refactored to use shared bfgs_common helpers; logic and caching pattern unchanged
nvmolkit/CMakeLists.txt Added _uffOptimization build target following existing _mmffOptimization pattern
nvmolkit/init.py Minor docstring update noting UFF support
src/minimizer/CMakeLists.txt Added bfgs_uff.cpp and bfgs_common.cpp source files

Reviews (1): Last reviewed commit: "Consolidate things" | Re-trigger Greptile

Comment on lines +7 to +9
vdwThresholds: Any = ...,
ignoreInterfragInteractions: Any = ...,
hardwareOptions: Any = ...,
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.

P2 Weak Any types reduce stub value

All three non-trivial parameters are typed as Any, eliminating IDE autocompletion and type-checker coverage for callers of the internal binding. The C++ layer already enforces concrete list types, so the stub can reflect them.

Suggested change
vdwThresholds: Any = ...,
ignoreInterfragInteractions: Any = ...,
hardwareOptions: Any = ...,
vdwThresholds: List[float] = ...,
ignoreInterfragInteractions: List[bool] = ...,
hardwareOptions: Any = ...,

Comment on lines +69 to +70
rdDistGeom.EmbedMultipleConfs(mol, numConfs=1, params=params)
conf = mol.GetConformer()
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.

P2 EmbedMultipleConfs return value not checked

EmbedMultipleConfs returns a list of assigned conformer IDs (empty on failure). If embedding fails, the unchecked mol.GetConformer() on the next line raises ValueError: No conformers present, making the test failure hard to diagnose.

Suggested change
rdDistGeom.EmbedMultipleConfs(mol, numConfs=1, params=params)
conf = mol.GetConformer()
confs_embedded = rdDistGeom.EmbedMultipleConfs(mol, numConfs=1, params=params)
if not confs_embedded:
raise RuntimeError("EmbedMultipleConfs failed for make_fragmented_mol(); check SMILES or random seed")
conf = mol.GetConformer()

Comment on lines +91 to +102
for (const auto& confInfo : batchConformers) {
const uint32_t numAtoms = confInfo.mol->getNumAtoms();
conformerAtomStarts.push_back(currentAtomOffset);
currentAtomOffset += numAtoms;

nvMolKit::confPosToVect(*confInfo.conformer, pos);
auto ffParams = constructForcefieldContribs(*confInfo.mol,
vdwThresholds[confInfo.molIdx],
confInfo.conformerId,
ignoreInterfragInteractions[confInfo.molIdx]);
addMoleculeToBatch(ffParams, pos, systemHost, metadata, confInfo.molIdx, static_cast<int>(confInfo.confIdx));
}
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.

P2 No per-molecule caching of FF params, unlike the MMFF counterpart

constructForcefieldContribs is called once per conformer. The MMFF implementation (bfgs_mmff.cpp) uses a moleculeCache keyed on ROMol* to call this only once per unique molecule per batch, since bond/angle/torsion/vdW topology parameters are conformer-independent. For batches containing molecules with many conformers, UFF re-derives the full force-field topology redundantly on every iteration.

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