-
Notifications
You must be signed in to change notification settings - Fork 11
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
Ram fix #134
Conversation
Now with an updated test suite, it could be verified that all tests succeed (only minor tolerance tweaking required). Especially tests regarding positional gradients run as is. Hence, ready to merge. |
I will take a look at it. |
Fixed the update-induced fails. |
Logs for tests where tolerances needed to be adapted. The first entry denotes the previous method, and the second one the new method. Output
|
It seems that the This should come from get_electronic_free_energy which is applied after SCF iterations. Hence, only different values in the |
I don't quite understand. If I look at the electronic and the electronic free energy, they are both different.
Doesn't that mean that some settings are not passed down properly anymore? |
I reverted them because these are intended changes (as discussed yesterday). We required pydantic>=2.0.0. Please do a |
Added test for checking unique electronic energy for isolated elements. Apparently, different energies are calculated. |
Easy fix. Fermi energy was not correctly added to |
Hessian test (test/test_scf/test_hess.py) is still failing for old tolerances
# The initial guess is an "arbitrary" tensor, and hence not part of AD computational graph. | ||
# NOTE: This leads to not entering xitorch._RootFinder.backward() at all during a | ||
# loss.backward() call. However, then the position tensor does receive gradient. | ||
guess = guess.detach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this causes the Hessian test to fail, since it discards the gradient of the EEQ guess?
Besides, if I comment this out, the test also throws an error: AttributeError: '_Data' object has no attribute 'occupation'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you comment guess.detach()
you can remedy the attribute error by commenting https://github.com/grimme-lab/xtbML/blob/8af902d5a50c9ad054f047e1ffbbbbb69491e2dc/src/dxtb/scf/data.py#L183
This leads to the behavior described in the comment, i.e. no Tensor enters xitorch._RootFinder.backward()
. This is expected, as there is currently no support for using AG w.r.t. to positions in the implicit
SCF implementation.
Manually merged into #132. |
The following changes have been made:
This allows to fulfill the provided RAM tests and counteract the existing memory leak.
Please review the changes and provide feedback.