-
Notifications
You must be signed in to change notification settings - Fork 459
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
additional XC superfunctionals #877
Conversation
Thanks! Yes, a single test file will be fine (unless multiple is more convenient for you). Just make a dir in |
I think what we need to do is organize the non-XC custom functionals into a series of tests like @susilehtola One more example why we need access to parameters in LibXC. |
Also @hokru you might want to check out the @bwb314 At some point you added many DFT LibXC tests against Q-Chem that missed the rebasing and was finally closed here. Do you still have these tests that we can add to Psi4? |
@dgasmith bad example, since these should go in libxc proper ;) |
@susilehtola @dgasmith They should maybe be included, yes, but exposing functional parameters to the python layer is a dream for functional development in general! |
@hokru as if the world needs more functionals :P |
cfe974c
to
515a2a4
Compare
d_pw6=-(bt-beta)/(X_FACTOR_C*X2S*X2S) | ||
f_pw6=1.0e-6/(X_FACTOR_C*X2S**expo_pw6) | ||
pw6.set_tweak([a_pw6,b_pw6, c_pw6, d_pw6, f_pw6, alpha_pw6, expo_pw6]) | ||
sup.add_x_functional(pw6) |
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 like lines 232-263 can be replaced by
sup.add_x_functional(core.LibXCFunctional('XC_GGA_X_MPW91', restricted))
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.
yes, but this custom mPWPW was basically just a test to understand what i need to do to modified pw91 for pwpb95. It will be removed. Further below is the mPWPW build using exactly what you suggest.
I added tests this way. I still want to add some reference outputs.
I am not, I also don't see it mentioned anywhere? I can join if that helps. |
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.
Thanks for all of these changes. Starting to look really good!
sup.set_c_alpha(1.0) | ||
sup.set_c_os_alpha(0.46) | ||
sup.set_c_ss_alpha(0.43) | ||
sup.set_c_ss_alpha(0.40) |
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.
Hmm, was this always wrong? It doesn't seem to flip a test.
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.
dsd-blyp was commented out. So i guess something was up.
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.
Ah ok, good to know it was never live :)
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.
IIRC, the SCS DH plus variants were alive c. beta3. Chris Cook had charge of them, as he was trying out the different -D parameterizations. Then SCS DH were disabled for years after DH were reformed. If there was any uncertainty, esp. in -D parameters, I? didn't uncomment them after the libxc reform. Glad the dust has settled on some of these DSD variations, and thanks for sorting them out, @hokru.
@@ -32,7 +32,6 @@ | |||
|
|||
from psi4 import core |
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.
Can you run YAPF
over this file and other python ones you have touched? You can install it with pip
.
# Tab in, trailing newlines | ||
sup.set_description(' PWPB95 SOS Double Hybrid XC Functional\n') | ||
# Tab in, trailing newlines | ||
sup.set_citation(' L. Goerigk, S.Grimme JCTC 7, 291-309, 2011 \n') |
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.
Can you we use "J. Chem. Theory Comput" instead of JCTC to keep ISO 4 consistency.
# No spaces, keep it short and according to convention | ||
sup.set_name('mPWPW') | ||
sup.set_description(' mPWPW GGA Exchange-Correlation Functional\n') | ||
sup.set_citation(' Adamo, C.; Barone, V. J. Chem. Phys. 1998, 108, 664 \n') |
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.
Extraneous semicolon here I think. We also want to keep consistent vol/page/year for citations.
# Tab in, trailing newlines | ||
sup.set_description(' PW6B95 Hybrid-meta XC Functional\n') | ||
# Tab in, trailing newlines | ||
sup.set_citation(' Zhao and Truhlar, J Phys Chem A., 109, 25, 2005, 5656-5667 \n') |
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.
Ditto on vol/page/year.
# No spaces, keep it short and according to convention | ||
sup.set_name('tpss') | ||
sup.set_description(' TPSS Meta-GGA XC Functional\n') | ||
sup.set_citation(' J. Tao, J. P. Perdew, V. N. Staroverov, and G. E. Scuseria Phys. Rev. Lett. 91 146401, 2003 \n') |
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.
Comma missing between vol/page and extra lines at end.
# => User-Customization <= # | ||
|
||
# No spaces, keep it short and according to convention | ||
sup.set_name('tpss') |
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.
Names should be upper case to match other (applies to all new funcs). Very surprised this doesn't come from LibXC, looks like they only give TPSSh.
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.
Well, that's one of the open questions about libxc standards that hasn't really been addressed. I think opinions vary, but so do functionals. TPSSh could follow other ab initio (non-fitted) functionals like most LDAs and GGAs and some meta-GGAs and have a separate exchange part and correlation part.
tests/dft-custom-gga/input.dat
Outdated
for func in ggalist[:]: | ||
edft = energy(func) | ||
label=func.upper() | ||
val=float(ggaval[func]) |
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.
Can be replaced with:
for func, val in ggaval.items():
edft = energy(func)
...
The ggalist
can be removed.
I added your requested changes @dgasmith, including running yapf. I did all my planned changes. Pending further review this is ready to go, i think. |
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.
Awesome! Thanks for all of new functionals and testing infrastructure.
@psi4/maintainers Lets get a few more eyes on this. |
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 great! Thanks for all the ref outputs from other codes.
tests/dft-custom-hybrid/input.dat
Outdated
# "wpbe0" : build_wpbe0_superfunctional, | ||
# "b5050lyp" : build_b5050lyp_superfunctional, | ||
|
||
psi4.core.set_output_file("output.dat", False) |
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.
Are these output file resets leftovers or still doing something?
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 took it initially from the libxc test. Wasn't sure if i needed to use it. Just delete them?
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.
Yes, delete them. That libxc is a psiapi test masquerading as psithon. Yours are normal psithon.
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.
ah ok! Will do so.
|
||
edft = energy('dldf+d09') | ||
val = -153.8046388583318276 #TEST | ||
compare_values(val, edft, 8, "dlDF+D09 (2009 +D correction)") #TEST |
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.
Are ref val
coming from psi runs of this input file? If so, the ref value only good to ~6 decimal places (default e_conv for energy), so checking to 8 won't be reliable. If this is the case, need to generate the val
with e_/d_conv 10 or so, save that to val
, then set e_/d_conv to 8 in this file, and continue checking to 8.
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.
All dldf related lines are copy & paste from the individual test (dft-dldf). Unknown to me when the ref values were generated.
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.
Ok, then we'll leave the ref as-is. May have to turn down the number of decimal places in test case in future.
@dgasmith and @bwb314, the libxc test case looks like the one mentioned #877 (comment) . Is it, or does that still need to be added? |
@loriab They are a different kind of test, there may be some overlap however. |
Description
Defines couple new XC functionals in the driver and adds few D3BJ parameters
Todos
Notable points that this PR has either accomplished or will accomplish.
User-Facing for Release Notes
Developer notes/issues
supersedes a number of older tests (removed only from CMakeLists.txt):
-
dft-dsd + dft-pbe0-2
moved intodft-custom-dhdf
-
dft-dldf
moved intodft-custom-mgga
Status