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

Support OpenFF Toolkit v0.11+ #198

Merged
merged 16 commits into from
Dec 12, 2022
Merged

Support OpenFF Toolkit v0.11+ #198

merged 16 commits into from
Dec 12, 2022

Conversation

Yoshanuikabundi
Copy link
Contributor

@Yoshanuikabundi Yoshanuikabundi commented Oct 4, 2022

Description

Update BespokeFit to work with new versions of the OpenFF Toolkit

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Add pins for Toolkit v0.11+
  • Transition to OpenFF Units
  • Update for breaking changes in 0.11+
  • Check optimize.err for errors in force balance handler
  • Set minimum versions of toolkit, qcsubmit, and qcengine once they are released (see status)

Status

@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #198 (78a3ea9) into main (3abdcca) will decrease coverage by 0.06%.
The diff coverage is 81.25%.

Additional details and impacted files

Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

The molecule in test_submit::test_submit_multi_molecule is failing because RDKitToolkitWrapper thinks Cu2+ is a radical. So the toolkit should be changed to accept Cu2+ so that tests here pass.

This also touches on the issue that QCSubmit sometimes puts multiple non-covalently-bound components in a single OFFMol, but while that's still something we need to get rid of some day, that's not the root cause here and so it shouldn't be a blocker to this PR.

So, accepted once the tests pass (which will probably involve merging a Toolkit PR)

@jthorton
Copy link
Contributor

jthorton commented Oct 6, 2022

@Yoshanuikabundi I think this test should work with any multi-molecule system maybe something like Cc1ccc(cc1Nc2nccc(n2)c3cccnc3)NC(=O)c4ccc(cc4)CN5CCN(CC5)C.CS(=O)(=O)O imatinib mesylate would do the job to save you waiting on a toolkit PR?

@Yoshanuikabundi
Copy link
Contributor Author

@jthorton Thanks but the Toolkit PR is already in :)

@Yoshanuikabundi
Copy link
Contributor Author

Looks like we also need leeping/forcebalance#259, which is currently in limbo. Tests for force balance pass, but trying to actually run an optimization fails. The error message is extremely cryptic and the true cause is difficult to lock down owing to a lack of optimize.err handling in ForceBalanceOptimizer._read_output() and the aggressive clean up of temporary files throughout the codebase. BespokeFit believes FB to have completed successfully when it actually errored out at an early stage, leading to a failure to find result outputs. I'll add in some code to handle this once I've got something ready for the BespokeFit workshop.

@mattwthompson mattwthompson mentioned this pull request Nov 15, 2022
@mattwthompson
Copy link
Member

Tests are starting to come in with some green checks - if anybody wishes to once it over again I'll give a window of a few days before merging (and another window for checks prior to making/pushing for a release).

@mattwthompson mattwthompson merged commit a4d7744 into main Dec 12, 2022
@mattwthompson mattwthompson deleted the toolkit_011_support branch December 12, 2022 19:55
@Yoshanuikabundi Yoshanuikabundi mentioned this pull request Jan 19, 2023
8 tasks
@Yoshanuikabundi Yoshanuikabundi added this to the v0.2.1 milestone Mar 16, 2023
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.

4 participants