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

Re-jig BCCs to be more specific #131

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Re-jig BCCs to be more specific #131

wants to merge 9 commits into from

Conversation

lilyminium
Copy link
Contributor

Description

Re-do OpenEye BCC collection

Status

  • Ready to go

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (a97eb10) 84.06% compared to head (f1c0957) 84.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   84.06%   84.05%   -0.02%     
==========================================
  Files          37       37              
  Lines        2021     2019       -2     
==========================================
- Hits         1699     1697       -2     
  Misses        322      322              
Impacted Files Coverage Δ
openff/recharge/charges/bcc.py 94.26% <100.00%> (-0.10%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lilyminium lilyminium marked this pull request as ready for review June 30, 2023 05:39
Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

Could you provide a little more context for these changes? I'm inclined to nearly rubber-stamp whatever changes you wish, but I'd like to have an idea what's going on here.

  • Is there any metric of correctness for these BCC SMIRKS? CI is all green after changing some patterns, which is not intuitive
  • How does openff/recharge/data/bcc/original-am1-bcc.json differ from openff/recharge/data/bcc/openeye-am1-bcc.json?
  • If I'm following the paper trail, scripts/convert-am1-bcc/generate_original_am1bcc.py is to be compared to the file now at scripts/convert-am1-bcc/legacy/convert.py? GitHub's UI doesn't pick up on this but I can just diff them locally.
  • There are some commented-out patterns in GENERAL_ATOM_CODES - intentional or temporary?

@lilyminium
Copy link
Contributor Author

The "correctness" is how well it corresponds to how OpenEye assigns things. There's a benchmark in scripts/validation, although it's too expensive to run in CI. The new parameters here do somewhat better than the original (and include phosphorus), but not perfectly. I'll see if I can a) improve the smirks a bit more and b) incorporate some of it into CI.

original-am1-bcc.json is what used to be there, it probably should be moved into a legacy file.

If I'm following the paper trail, scripts/convert-am1-bcc/generate_original_am1bcc.py is to be compared to the file now at scripts/convert-am1-bcc/legacy/convert.py? GitHub's UI doesn't pick up on this but I can just diff them locally.

That's right :)

There are some commented-out patterns in GENERAL_ATOM_CODES - intentional or temporary?

These were intentionally commented out to leave a bit of a trail in case the more general patterns should be reverted back to the stricter ones... thanks to your feedback I've realised this PR as a whole is probably still a bit premature.

@lilyminium lilyminium marked this pull request as draft July 4, 2023 08:58
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.

None yet

2 participants