-
Notifications
You must be signed in to change notification settings - Fork 149
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
Adds output of Hessians to xtb when acting as an external program for Gaussian 16 #363
base: main
Are you sure you want to change the base?
Conversation
In addition, I'd be very happy if there was an "easy" way to control runtype based on the xtb/src/io/reader/gaussian.f90 Line 47 in 7e8e21c
which is set by Gaussian to 1 for gradient and 2 for Hessian evaluation. I've considered calling set_runtyp in the reading subroutine but that's maybe a bit too much global state manipulation. The alternative I can see is to create an entirely new "under gaussian control" runtype.
Very interested in your opinion on this, thanks. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
==========================================
- Coverage 40.80% 40.77% -0.04%
==========================================
Files 304 304
Lines 50774 50813 +39
==========================================
Hits 20718 20718
- Misses 30056 30095 +39 ☔ View full report in Codecov by Sentry. |
The read mode could be cached in the molecular structure type and the main routine can than decide if it is able to invoke flag=$(awk 'NR == 1 {if ($2 == 2) {print "--hess"} else {print "--grad"} }' $2) |
Thanks for this and you are right. awk is the right tool to use here and will allow me to remove any final Perl dependencies, moving the whole thing to a self contained two line Bash script. Will update (and sign off commits! Sorry again) Should I add a unit test too? Without Gaussian of course, just testing file io structure. |
We don't have unit tests for the frequency calculation yet, since it is a bit costly in the CI. I'll have a look myself while checking the PR. |
Signed-off-by: Cyrille Lavigne <cyrille.lavigne@mail.utoronto.ca>
Signed-off-by: Cyrille Lavigne <cyrille.lavigne@mail.utoronto.ca>
Signed-off-by: Cyrille Lavigne <cyrille.lavigne@mail.utoronto.ca>
Signed-off-by: Cyrille Lavigne <cyrille.lavigne@mail.utoronto.ca>
4e73b63
to
7046085
Compare
I made all the suggested changes. The script with awk works beautifully, and the performance is much better than by passing through perl or python to do the required Hessian reformatting. Thanks for your help! |
Overall it still looks a bit hacky, but as long as it only triggers with Gaussian input we can be sure it won't mess up regular |
I agree it is pretty hacky. I don't need the feature right now, as I can just compile my PR branch myself, so if you think it's best to refactor instead, I'm open to it. |
@clavigne Is this feature still relevant? If yes, would you be open to implement it into the current code? |
This is a feature improvement related to #329 .
Basically, I've added Hessian output capability for xtb acting as an external program invoked by Gaussian 16. I've made the changes to
writeResultsGaussianExternal
that are necesserary for the Hessian output.This would have been otherwise extremely straightforward, except for the fact that the raw Hessian matrix (before mass-weighting, which is what Gaussian expects) is not saved in
numhess
for (I assume) memory usage reasons. I didn't want to just have the code keep the raw Hessian every time for the vast majority of users not running under Gaussian, so I short-circuitednumhess
instead to immediately return when the input file is a Gaussian external file. (This additionally has the advantage that it avoids an extraneous Hessian diagonalization by xtb, which gets expensive for large molecules.)This solution is far from elegant as it required adding some more global state. Feel free to suggest an alternative.