Skip to content

Add custom options for MMFF optimization#124

Open
scal444 wants to merge 5 commits intoNVIDIA-Digital-Bio:mainfrom
scal444:new_pr4_mmffopts
Open

Add custom options for MMFF optimization#124
scal444 wants to merge 5 commits intoNVIDIA-Digital-Bio:mainfrom
scal444:new_pr4_mmffopts

Conversation

@scal444
Copy link
Copy Markdown
Collaborator

@scal444 scal444 commented Mar 31, 2026

Allows better control over dielectric, interfrag interactions, and nonbond interaction limit. Partial implementation of #70

@scal444 scal444 requested a review from evasnow1992 March 31, 2026 16:03
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 31, 2026

Greptile Summary

This PR extends MMFFOptimizeMoleculesConfs with fine-grained control over the MMFF force field: callers can now supply an MMFFMolProperties object (or a per-molecule list), a scalar or per-molecule nonBondedThreshold, and ignoreInterfragInteractions. The Python–C++ bridge pattern correctly extracts only the configuration settings from the RDKit properties object (variant, dielectric, term toggles) and lets the C++ side derive all molecule-specific atom-type data from the ROMol* directly, so broadcasting a single properties object across multiple molecules is intentional and safe.

Key changes:

  • mmffOptimization.py — three new public parameters with _normalize_scalar_or_list / _normalize_properties helpers that handle scalar-vs-sequence and single-object-vs-per-molecule cases.
  • mmffOptimization.cppextractMMFFProperties / extractMMFFPropertiesList unpack the Python transport struct into nvMolKit::MMFFProperties; the old scalar nonBondedThreshold argument is fully replaced by the properties list.
  • _mmffOptimization.pyi — stub updated; the declared default (None) is a minor mismatch vs the actual C++ default (empty list), flagged below.
  • Tests — three well-constructed new integration tests including a fragmented-molecule fixture (now with a fixed random seed) and per-molecule property/threshold/interfrag combinations.

Confidence Score: 5/5

  • Safe to merge — no functional bugs found; the single finding is a minor stub inconsistency in an internal module.
  • All logic, validation, and bridging code is correct. The only issue found (P2) is that _mmffOptimization.pyi advertises None as the default for properties while the C++ binding uses an empty list. This has no runtime impact on the public API since the Python wrapper always supplies an explicit list. No P0 or P1 issues remain.
  • nvmolkit/_mmffOptimization.pyi — minor stub default mismatch.

Important Files Changed

Filename Overview
nvmolkit/mmffOptimization.cpp Replaces scalar nonBondedThreshold with a per-molecule properties list; adds extractMMFFProperties and extractMMFFPropertiesList helpers that unpack the Python transport object into nvMolKit::MMFFProperties. Size validation is sound.
nvmolkit/mmffOptimization.py Public API adds properties, nonBondedThreshold, and ignoreInterfragInteractions parameters; normalisation helpers correctly handle scalar-vs-sequence and single-vs-list MMFFMolProperties. Logic for broadcasting a single config object to all molecules is intentional (nvMolKit derives atom-specific parameters from ROMol* in C++, not from the properties object).
nvmolkit/_mmffOptimization.pyi Stub updated to reflect new properties parameter, but the declared default (None) does not match the C++ binding default (empty boost::python::list()); passing None directly to the extension would raise a Boost.Python TypeError.
nvmolkit/tests/test_mmff_optimization.py Three new tests cover custom scalar properties, per-molecule properties, and per-molecule threshold/interfrag flags. make_fragmented_mol now uses a fixed seed (42) addressing the prior review comment. make_rdkit_mmff_properties correctly calls capture_mmff_settings so the bridge can extract config settings without relying on RDKit getter availability.
nvmolkit/types.py Trivial formatting change — two blank lines added before the _embedMolecules import. No functional changes.

Reviews (2): Last reviewed commit: "Format" | Re-trigger Greptile

)
for props, threshold, ignore_interfrag in zip(properties_list, thresholds, interfrag_flags)
]
if len(native_properties) == 1:
Copy link
Copy Markdown
Collaborator

@evasnow1992 evasnow1992 Mar 31, 2026

Choose a reason for hiding this comment

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

Is there any specific reason we want to extract this single property instead of just passing in it as a list of one element?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It matters for which src cpp path we take - https://github.com/NVIDIA-Digital-Bio/nvMolKit/blob/main/src/minimizer/bfgs_mmff.h#L38-L44, but you're right we didn't need it at this level. Fixed

Copy link
Copy Markdown
Collaborator

@evasnow1992 evasnow1992 left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Thanks!

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.

2 participants