Skip to content

Expose FIRE minimizer Python APIs#225

Open
scal444 wants to merge 4 commits into
NVIDIA-BioNeMo:mainfrom
scal444:fire_pr_6
Open

Expose FIRE minimizer Python APIs#225
scal444 wants to merge 4 commits into
NVIDIA-BioNeMo:mainfrom
scal444:fire_pr_6

Conversation

@scal444

@scal444 scal444 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

PR 6 for #216

@scal444
scal444 requested a review from evasnow1992 July 9, 2026 17:45
@scal444 scal444 self-assigned this Jul 9, 2026
Comment thread nvmolkit/uffOptimization.cpp Outdated
Comment thread nvmolkit/mmffOptimization.py
@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR exposes the FIRE energy minimizer as a first-class Python API for both the standalone (MMFFOptimizeMoleculesConfs, UFFOptimizeMoleculesConfs) and batched-forcefield (MMFFBatchedForcefield.minimize, UFFBatchedForcefield.minimize) code paths. It introduces a new _types Boost.Python module that wraps FireOptions, re-exports it through types.py, and wires it into all four minimization entry points with Python-level validation and C++ routing via a local MinimizerKind enum.

  • A new _types module (types.cpp / _types.pyi) exposes FireOptions with all tuneable fields; types.py re-exports it as a public symbol.
  • minimizerKind and fireOptions parameters are added to the MMFF and UFF standalone functions and the shared _BatchedForcefieldBase._minimize() method, with Python-level validation before forwarding to C++.
  • Both stubs for the *ConfsDevice variants (_mmffOptimization.pyi and _uffOptimization.pyi) mark several required C++ parameters as optional — those args have no Boost.Python default, so calling the private C++ module directly without them will raise a runtime error.

Confidence Score: 5/5

The public Python API and C++ routing logic are correct; the only rough edges are in the private _mmffOptimization and _uffOptimization stub files.

All changes to the public-facing Python wrappers and C++ bindings are consistent and well-tested. The stub inaccuracies affect only private underscore modules that end-users are not expected to call directly, and the Python wrappers always supply all required arguments explicitly before delegating to those modules.

nvmolkit/_mmffOptimization.pyi and nvmolkit/_uffOptimization.pyi — the newly added ConfsDevice stubs show required C++ parameters as optional.

Important Files Changed

Filename Overview
nvmolkit/types.cpp New Boost.Python module exposing FireOptions struct; all fields correctly wired with def_readwrite.
nvmolkit/_types.pyi New stub for _types module; all FireOptions fields listed, __init__ correctly annotated.
nvmolkit/types.py Re-exports FireOptions from the new _types C++ module via noqa: F401; minimal and correct.
nvmolkit/_mmffOptimization.pyi New MMFFOptimizeMoleculesConfsDevice stub incorrectly marks maxIters, properties, hardwareOptions, and targetGpu as optional; those are required in the C++ binding.
nvmolkit/_uffOptimization.pyi New UFFOptimizeMoleculesConfsDevice stub marks five required C++ args as optional; fireOptions: object is inconsistent with the MMFF stub (Any).
nvmolkit/mmffOptimization.cpp Adds FIRE path to both MMFFOptimizeMoleculesConfs and MMFFOptimizeMoleculesConfsDevice; backend/minimizer parsing extracted into local helpers; C++ defaults added to MMFFOptimizeMoleculesConfs binding.
nvmolkit/uffOptimization.cpp Mirrors MMFF changes for UFF; FIRE path added; hardwareOptions given a default in the UFFOptimizeMoleculesConfs binding.
nvmolkit/batchedForcefield.cpp FIRE path added to all four minimize methods; fireOptionsWithGradTol helper always overwrites gradTol with the forceTol parameter (previously flagged).
nvmolkit/batchedForcefield.py Adds minimizerKind / fireOptions to _BatchedForcefieldBase._minimize() and propagates them through both MMFFBatchedForcefield and UFFBatchedForcefield overloads with proper validation.
nvmolkit/mmffOptimization.py Adds backend, minimizerKind, fireOptions to the public MMFFOptimizeMoleculesConfs wrapper with Python-level validation and alias normalisation before forwarding to C++.
nvmolkit/uffOptimization.py Adds minimizerKind / fireOptions to UFFOptimizeMoleculesConfs; no backend parameter (UFF FIRE uses a fixed backend internally, asymmetric with MMFF but consistent with the C++ binding).
nvmolkit/tests/test_mmff_optimization.py Adds FIRE correctness tests (BATCHED and PER_MOL backends) plus validation tests for unknown minimizerKind and backend; coverage looks adequate.
nvmolkit/tests/test_uff_optimization.py Adds FIRE correctness test and minimizerKind rejection test for the UFF path; matches the MMFF test structure.
nvmolkit/tests/test_batched_forcefield_device.py New test_minimize_fire_host_returns_nested_energy_and_status covers MMFF and UFF batched-forcefield FIRE path; checks shape and type of returned tensors.
nvmolkit/tests/test_types.py New test_fire_options_exposes_native_defaults_and_mutators verifies default gradTol and field mutability; straightforward and correct.
nvmolkit/CMakeLists.txt Adds _types CMake target for the new types.cpp module; structure matches existing targets.

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

Comment on lines +440 to +453
minimizer_kind = str(minimizerKind).upper()
if minimizer_kind not in {"BFGS", "FIRE"}:
raise ValueError("minimizerKind must be 'BFGS' or 'FIRE'")
if fireOptions is None:
fireOptions = FireOptions()
if output == CoordinateOutput.DEVICE:
return self._native_ff.minimizeDevice(maxIters, forceTol, -1 if target_gpu is None else int(target_gpu))
energies, converged = self._native_ff.minimize(maxIters, forceTol)
return self._native_ff.minimizeDevice(
maxIters,
forceTol,
-1 if target_gpu is None else int(target_gpu),
minimizer_kind,
fireOptions,
)
energies, converged = self._native_ff.minimize(maxIters, forceTol, minimizer_kind, fireOptions)

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.

P1 forceTol silently overrides user-supplied fireOptions.gradTol

In the C++ binding (batchedForcefield.cpp), the FIRE path calls fireOptionsWithGradTol(fireOptions, gradTol), which copies the user's FireOptions and overwrites its gradTol with the forceTol parameter (default 1e-4). Any explicit fireOptions.gradTol the caller sets is therefore silently discarded. The new test illustrates this: it sets options.gradTol = 1e-3 and calls minimize(maxIters=100, minimizerKind="FIRE", fireOptions=options) — but forceTol defaults to 1e-4, so the test is unknowingly running with a different tolerance than intended. Neither the minimize() docstring nor the FireOptions.gradTol field documents this override.

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