Add fix for charge assignment fallback and test#1262
Draft
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1262 +/- ##
==========================================
- Coverage 93.86% 93.80% -0.06%
==========================================
Files 72 72
Lines 6240 6246 +6
==========================================
+ Hits 5857 5859 +2
- Misses 383 387 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
j-wags
added a commit
that referenced
this pull request
Jul 17, 2025
Member
Author
|
I forgot this PR existed and made another push at this here: #1206 (comment) |
Merged
mattwthompson
added a commit
that referenced
this pull request
Aug 19, 2025
* initial implementation of NAGLChargesHandler * have testing env use naglcharges toolkit branch * adding a bunch of tests, some todos remain * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * I guess valueerror is fine * update vsite charge test * Apply suggestions from code review Co-authored-by: Matt Thompson <mattwthompson@protonmail.com> * Replace usages of Interchange.from_smirnoff with ForceField.create_interchange * remove repeated nagl FF creation and replace with new fixture * add check that charge_from_molecules takes precedence over NAGLCharges * Tighten all total charge tolerances in new tests down to 1e-10 * add test for NAGL charge assignment failure falling back to lower precedence charge model that CAN assign charges * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Implement correct(er) error handling/reporting behavior for NAGLCharges * test new error handling logic * test with new openff-nagl-models * remove fallback behavior test since it's being handled separately in #1262 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * use a more recent nagl model in host-gues example in case that helps prevent fetch? * don't monkeypatch get_release_metadata (since it doesn't exist any more) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add fail fast and strip down test matrix * try getting openff-nagl-models from main branch * better check for NAGL wrapper in registry * add tests for naglcharges being superseded by charge_from_molecules * rename existing sage_with_nagl fixture in charge assignment logging tests to sage_with_nagl_chargeincrements to differentiate from new NAGLChargesHandler * add naglcharges speed tests * add charge assignment logging tests and update strings with NAGL behavior and tests * improve test documentation and consolidate invalid model name tests * implement dangerous logic for charge method fallback handling and error output, and test * remove dangerously broad charge handler fallback and mark tests as xfail * Lint and various cleanups/import rearrangement * restore testing matrix * relax performance test runtime * add link to charge fallback test skip reasoning * revert unrelated changes * update openff-nagl-models branch used for test envs * try not installing from branches in docs env * have docs env fetch dev branches of toolkit and nagl-models * Increase allowed execution time on nagl runtime tests * route all charge assignment through cached _compute_partial_charges method * remove toolkitam1bcc from sage_with_nagl_charges test fixture * rename sage_with_nagl_charges test fixture to sage_nagl * remove unnecessary ToolkitAM1BCCHandler registration+deregistration from CIMH test * point to main branch of nagl-models now that PR is merged * Drop development versions * clean up pre-merge * update releasehistory --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Matt Thompson <mattwthompson@protonmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Provide a brief description of the PR's purpose here.
Checklist