-
Notifications
You must be signed in to change notification settings - Fork 159
Add explanation for translation entropy constants #1077
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
Conversation
So I rewrote the translation entropy equation and it gives approximately the same result (free energy differs by ~10^-5 kcal/mol) If I work out the equivalent of magic4 and magic5, they come out to:
As best I can tell, these must have been originally been worked out with slightly different definitions of the constants. Not all the constants used in xTB correspond to their exact current value. For example in thermo, these values are defined:
@marcelmbn Not sure if this is an acceptable difference and tests should be updated or this is too large a change and we just stick with the current magic constants, just providing some documentation on how they were likely calculated. |
Thanks for the correct implementation of the constant!
Thank you in advance for your work on this project! |
@TyBalduf Just wanted to ping you again on this topic. We'd be very grateful if you find the time to update the related points in the code to the SotA standard you suggested, including the correct ab initio constants, so that we can integrate everything into the |
I've been a little preoccupied of late and won't have a chance to work on this too soon. I can probably get to it eventually, its mainly a matter of updating tests. But I'm not actively working on it so if someone else wanted to take this on, there wouldn't be conflicting work. |
I updated the necessary constants from CODATA 2018 to be consistent with mctc-lib and calculated all necessary quantities directly from the fundamental constants. The resulting corrections change by about 10e-5 but the new values are just more accurate. Therefore, I adapted the tests to the new values. Should now be ready. |
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.
Looks good to me besides the few questions regarding constant relationships.
src/thermo.f90
Outdated
real(wp),parameter :: conv4 = 2.2868d0 ! approx R*ln(10)/2 | ||
real(wp),parameter :: conv5 = 2.3135d0 ! approx R*(ln[(kb/P°)*(2pi * kB * amutokg/h)^(3/2)] + 5/2) |
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.
I'd suggest deleting these lines as they should not be needed anymore.
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.
Then we have to remove the full routine, which I did. I think it mainly calculates everything in wavenumbers and was never used in the code.
! Alternative form: | ||
! s_tr_old=magic4*(5.0_wp*log10(t)+3.0_wp*log10(molmass))-magic5 | ||
! with: | ||
! magic4 = R*ln(10)/2 approx 2.2868d0 | ||
! magic5 = R*(ln[(kb/P°)*(2pi * kB * amutokg/h)^(3/2)] + 5/2) approx 2.3135d0 |
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.
We could also delete these old legacy parts but might also be helpful for some point in the future where it possibly helps somebody in understanding the equations? I'd leave it up to you if we keep it or not.
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.
I would keep this to explain the formula above. It is not directly obvious and long-term we need to clean up the thermo routine anyway. Then it is nice to have as much information as possible.
src/mctc/convert.f90
Outdated
real(wp), public, parameter :: fstoau = 41.3413733365614_wp | ||
real(wp), public, parameter :: autofs = 1.0_wp / fstoau |
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.
Shouldn't we replace these two lines with dependencies on the atomic_unit_of_time
? Both represent a relation between units of time (SI vs atomic units)?
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.
(Or the other way around would obviously also be possible)
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.
We can do this. I used the new value which comes from mctc-lib (our general goal should be to consistently use only mctc-lib values for all constants, as this will replace the internal mctc routines in xtb).
Signed-off-by: Ty Balduf <ty.balduf@schrodinger.com>
Signed-off-by: Ty Balduf <ty.balduf@schrodinger.com>
Signed-off-by: thfroitzheim <92028749+thfroitzheim@users.noreply.github.com>
Signed-off-by: Marcel Müller <marcel.mueller@thch.uni-bonn.de>
Worked out the constants following the expressions for translation entropy given (among other places) here