Repository navigation
Reset component caches when forces get a new positions tensor - #278
Open
thebreadishard wants to merge 2 commits into
Open
thebreadishard wants to merge 2 commits into
thebreadishard wants to merge 2 commits into
Conversation
The classical, interaction and integral caches are validated by the values of the positions only. A second positions tensor with equal values - for example after energy(), which builds the caches without a graph, or a second forces() call with a fresh tensor - reused caches tied to another tensor, so autograd missed their dependence on the new positions and the forces were wrong (water, GFN2: z(O) +0.0755 instead of -0.0030 E_h/bohr). requires_positions_grad now clears those caches when the positions tensor is not the one the last graph was built for (tracked by a weak reference). Calls with the same tensor keep using the caches.
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
=======================================
Coverage 88.38% 88.38%
=======================================
Files 215 215
Lines 10784 10791 +7
Branches 1241 1242 +1
=======================================
+ Hits 9531 9538 +7
Misses 902 902
Partials 351 351 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
|
Thanks! Can this be merged? |
This branch was successfully deployed
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
2 participants
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.
Fixes #277.
requires_positions_gradnow keeps a weak reference to the positions tensor that the last graph was built for. Whenforces(oranother method behind that decorator) gets a different tensor, it first clears the classical, interaction and integral caches. These are
the caches
Calculator.reset()clears, apart from the result cache, which is already keyed on the tensor id. Calls with the same tensorkeep using the caches, so repeated derivatives of one graph are not slowed down.
Test:
test/test_calculator/test_cache/test_positions_graph.pycovers forces afterenergy, andforcestwice with a new tensor,for GFN1 with the PyTorch driver and, when libcint is available, for GFN1 and GFN2 with libcint. All 6 cases fail on
main(2070660) andpass on this branch.
Other tests run locally (Python 3.11, torch 2.14.1 CPU, the tad versions
mainpins, tad-libcint 0.3.0):test/test_calculator: 60 passedtest/test_singlepoint/test_grad_gfn1.py: 42 passedtest/test_singlepoint/test_grad_gfn2.py: 18 passedtest/test_singlepoint/test_hess.py: 3 passedtest/test_properties/test_hessian.py: 7 passedtest/test_properties/test_vibration.py: 3 passedtest/test_properties/test_dipole_deriv.py: 8 passedThe rest of the suite was not run.
🤖 Generated with Claude Code