Skip to content

feat: add bond length distribution benchmark - #666

Open
lwalew wants to merge 6 commits into
ddmms:mainfrom
lwalew:feat/add-bond-length-distribution
Open

feat: add bond length distribution benchmark#666
lwalew wants to merge 6 commits into
ddmms:mainfrom
lwalew:feat/add-bond-length-distribution

Conversation

@lwalew

@lwalew lwalew commented Jul 3, 2026

Copy link
Copy Markdown

Pre-review checklist for PR author

PR author must check the checkboxes below when creating the PR.

Summary

Migrates the bond length distribution benchmark from the MLIP Audit suite. For each of a set of small organic molecules — covering the C-C, C=C, C#C, C-N, C-O, C=O and C-F bond types — an NVT molecular dynamics simulation is run at 300 K starting from a QM-optimised reference geometry (selected from QM9), and the deviation of a tracked covalent bond from its reference length is measured over the trajectory. A well behaved potential keeps bonds close to their reference length, so a lower average deviation is better.

The analysis reports the average bond length deviation per model in the metrics table and shows the distribution of sampled deviations as a histogram.

Linked issue

Resolves #671

Progress

  • Calculations
  • Analysis
  • Application
  • Documentation

Testing

Not yet run end-to-end: the benchmark input data still needs to be uploaded to the ML-PEG S3 bucket at inputs/molecular_dynamics/bond_length_distribution/bond_length_distribution.zip (layout bond_length_distribution/bond_length_distribution.json). Kept as a draft until the data is available.

Verified locally in the meantime:

  • pre-commit passes on all changed files (ruff check, ruff format, numpydoc-validation).
  • Both test modules are collected by pytest with mlipaudit installed, and skip cleanly with Please install mlipaudit extra when it is not, rather than erroring during collection.
  • The element info written to info.json was checked against the dataset: 8 molecules, and the union of the per-molecule element lists is ['C', 'F', 'H', 'N', 'O'], matching BondLengthDistributionBenchmark.required_elements.
  • The calculation → analysis hand-off was exercised end-to-end with a local copy of the dataset, confirming the analysis reads the dataset from the calculation outputs without downloading it again.

New decorators/callbacks

No new callbacks required; uses the existing plot_hist and build_table decorators and the plot_from_table_column callback.

Note for reviewers

Addressing the review feedback here and the equivalent points on #658:

  • Element filtering. Follows the Folmsbee template, with elements saved as one list per molecule rather than a single union, so partial filtering can exclude individual molecules later. Ordering follows the dataset, which matches the molecule order in analyze()'s results. This matters here: only 1 of the 8 molecules contains F, so under a single union list, filtering out F would exclude the whole benchmark rather than one molecule.
  • Reusing the calculation output. The calculation now copies the downloaded dataset into its outputs, and the analysis reads it from there with data_input_dir=CALC_PATH, so it no longer re-downloads from S3 (previously twice per run). A missing dataset raises a clear "Please run the calculation" error.
  • pytest.importorskip. Added to both the calculation and analysis modules, positioned ahead of the ml_peg.calcs.utils.mlipaudit import, since that module imports mlipaudit unconditionally.
  • GPU cost estimate. Added to the docs, per the comment thread below.
  • Unlike Folmsbee and tautomers, this benchmark does not use the mock output as its source of truth. Those need it to order parity-plot hoverdata labels; the histogram here has no per-molecule labels, so the dataset in the calculation outputs is sufficient.
  • A model whose simulations all fail is reported as NaN rather than None.

@lwalew lwalew mentioned this pull request Jul 8, 2026
5 tasks
@ElliottKasoar ElliottKasoar added the new benchmark Proposals and suggestions for new benchmarks label Jul 8, 2026
Computational cost
------------------

High: one MD simulation per molecule, each 1,000,000 steps. Faster inference can be achieved

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

gpu estimate?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There are 8 molecules of size 4-13 atoms, each doing 1M steps so for instance for UMA-S's throughput of 1.4ns/day/1000 atoms on an H100, we can expect at least 2h for instance. Although there will be some overhead associated with the simulation engine so for most models, a couple of hours is realistic.

@joehart2001

Copy link
Copy Markdown
Collaborator

Thanks for the pr! same as #658 could you send the data + have a look at the element filtering?

docs: https://ddmms.github.io/ml-peg/developer_guide/filter.html

@ElliottKasoar ElliottKasoar added the external Contribution from external framework label Jul 23, 2026
Migrate the bond length distribution benchmark from the MLIP Audit suite.
An NVT molecular dynamics simulation is run for each of a set of small organic
molecules, and the deviation of a tracked covalent bond from its QM-optimised
reference length is measured over the trajectory. Lower average deviation is
better.

Adds the calculation, analysis (histogram of deviations plus a metrics table),
Dash app, documentation, and the mlipaudit dependency wiring (optional extra,
git source and MLIP Audit framework badge).

# Conflicts:
#	docs/source/user_guide/benchmarks/molecular_dynamics.rst
#	ml_peg/app/utils/frameworks.yml
#	ml_peg/calcs/utils/mlipaudit.py
@lwalew
lwalew force-pushed the feat/add-bond-length-distribution branch from 871ede0 to 1fefd71 Compare August 3, 2026 08:01
@lwalew
lwalew marked this pull request as ready for review August 3, 2026 08:13
lwalew and others added 4 commits August 3, 2026 10:17
Save the downloaded dataset to the calculation outputs and read it from there
during analysis, so the analysis no longer re-downloads the input data from S3.
Raise a clear error if the calculation has not been run.

Store elements as one list per molecule, in dataset order, so individual
molecules can be excluded once partial filtering is supported.

Report a failed model as NaN rather than None, and add a GPU cost estimate to
the docs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external Contribution from external framework new benchmark Proposals and suggestions for new benchmarks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New benchmark Bond Length Distribution

3 participants