Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Begin setting up torsion automation #13

Merged
merged 48 commits into from
Dec 17, 2024
Merged

Begin setting up torsion automation #13

merged 48 commits into from
Dec 17, 2024

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Nov 15, 2024

Development checklist

  • Update YAMMBS to use more than 24 cores at once when running torsion optimizations (might be useful to use fewer CPU cores until then)
  • Add plotting script - do next week
  • Other metrics? - do in the future

Submission Checklist

  • Created a new directory in the submissions directory containing the YDS input file and optionally a force field .offxml file
  • Triggered the benchmark workflow with a PR comment of the form /run-benchmark path/to/submission
  • Waited for the workflow to finish and a comment with Job status: success to be posted
  • Reviewed the results committed by the workflow
  • Published the corresponding Zenodo entry and retrieved the DOI
  • Added the Zenodo DOI to the table in the main README
  • Ready to merge!

@mattwthompson mattwthompson mentioned this pull request Nov 15, 2024
13 tasks
@ntBre ntBre mentioned this pull request Nov 21, 2024
@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/tmp.yaml

1 similar comment
@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/tmp.yaml

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/tmp.yaml

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/tmp.yaml

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/tmp.yaml

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/larger/tmp.yaml devtools/torsions.yaml

Copy link

A workflow dispatched to run torsion benchmarks for this PR has just finished.

Copy link

A workflow has been dispatched to run the benchmarks for this PR.

  • Run ID: 12306191378
  • Triggering actor: github-actions[bot]
  • Target branch: run-torsions

Copy link

A workflow dispatched to run torsion benchmarks for this PR has just finished.

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/larger/tmp.yaml devtools/torsions.yaml

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/larger/tmp.yaml devtools/torsions.yaml

Copy link

A workflow has been dispatched to run the benchmarks for this PR.

  • Run ID: 12318309008
  • Triggering actor: github-actions[bot]
  • Target branch: run-torsions

Copy link

A workflow dispatched to run torsion benchmarks for this PR has just finished.

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/larger/tmp.yaml devtools/torsions.yaml

1 similar comment
@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/larger/tmp.yaml devtools/torsions.yaml

Copy link

A workflow has been dispatched to run the benchmarks for this PR.

  • Run ID: 12319223858
  • Triggering actor: github-actions[bot]
  • Target branch: run-torsions

Copy link

A workflow dispatched to run torsion benchmarks for this PR has just finished.

@mattwthompson
Copy link
Member Author

/run-torsions torsion-dev/larger/tmp.yaml devtools/torsions.yaml

@mattwthompson
Copy link
Member Author

I don't think we took notes on this, so -

last week, @ntBre and I discovered that some data points (maybe 5-10%) were reporting both "no qm points" and "no mm points" which was a surprise because the molecules themselves were well-formed and processed into inputs. The problem was that YAMMBS looked up IDs by (mapped) SMILES alone, which works fine for molecules but doesn't work for torsion drives. Because a torsion drive can drive a given mapped SMILES at different atom quartets, the mapped SMILES alone cannot uniquely identify an ID and the lookup needs to also pass the dihedral indices through. See openforcefield/yammbs@b7df922

Copy link

A workflow has been dispatched to run the benchmarks for this PR.

  • Run ID: 12361612232
  • Triggering actor: github-actions[bot]
  • Target branch: run-torsions

Copy link

A workflow dispatched to run torsion benchmarks for this PR has just finished.

@mattwthompson mattwthompson marked this pull request as ready for review December 16, 2024 22:55
@mattwthompson mattwthompson requested a review from ntBre December 16, 2024 22:55
@mattwthompson
Copy link
Member Author

@ntBre could you give this a once-over (or a thorough review if you wish) and merge?

The only thing I'm not sure about is what the current torsion-dev/ directory should look like

@ntBre
Copy link
Collaborator

ntBre commented Dec 16, 2024

Sure, I'll take a look first thing tomorrow. I actually deleted my _tests directory (analogous to torsion-dev) when I finished testing in the earlier optimization tests, but I think it's also useful to keep it around and the current state looks fine to me.

Copy link
Collaborator

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

LGTM! The biggest question to me is whether env.yaml can be updated to point to a new yammbs release and torsions.yaml possibly deleted. The README and possible git issue are lower priority, but I approve nonetheless.

Comment on lines +32 to +34
if (comment.body.startsWith('/run-optimization-benchmarks')) {
benchmark_workflow = "opt.yaml"
} else if (comment.body.startsWith('/run-torsion-benchmarks')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The switch from run-benchmark to run-optimization-benchmarks and the new run-torsion-benchmarks could be added to the main README, but it's fine if that's not included in this PR.

Comment on lines 153 to 154
git commit -m "Add benchmark results"
git push
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't strictly related to the PR, but I realized last night that kicking off an optimization benchmark and a torsion benchmark at the same time in the same PR would probably cause one of them to fail at git push. One (or both) of them probably need a git pull --rebase before the push. Or that workflow could just not be supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I've opened a separate issue because I don't want to fix that right now

@@ -22,4 +22,4 @@ dependencies:
- tqdm

- pip:
- git+https://github.com/openforcefield/yammbs.git@0.1.1
- git+https://github.com/openforcefield/yammbs.git@add-torsion-models
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to update this to the latest yammbs release before merging?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

Copy link
Collaborator

Choose a reason for hiding this comment

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

And is this env still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope!

@mattwthompson
Copy link
Member Author

Thanks!

@mattwthompson mattwthompson merged commit b2cd18b into master Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants