-
Notifications
You must be signed in to change notification settings - Fork 7
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
Added additional support for Psi4 #12
Conversation
- Modified input Template to have header and memory input - Implemented basic handling of specifications for each calculation type - Modified handle_comand line function - Implemented handling multi specifications for calctype = opt+freq Additional structure file Added h2o.xyz file for tests Added tests - tests for additional calculation types - test for specifications - test for handling template with additional parameters Committing the rest of files
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.
Nice work. There are some little details to settle, but this seems good overall.
# CalcType.TS, | ||
# CalcType.OPTFREQ, | ||
CalcType.OPTFREQ: ["Opt+Freq Calculation", "optfreq", "opt+freq"], | ||
} |
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.
CALC_TYPES
is more to define the keywords for each calculation type and be able to get them programmatically (and make your job easier), rather than to detect synonyms. Detecting the calculation type is already handled by get_abs_type
in utilities.py
. The calculation type is already as CalcType
enum.
"software": "psi4", | ||
"method": "b3lyp", | ||
"basis_set": "6-31G", | ||
"specifications": "opt(return_wfn=True) freq(dertype=1 return_wfn=True)", |
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.
What happens if the specifications are opt(return_wfn=True) dertype=1, return_wfn=True
?
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.
It will generate an output with only specifications for the "opt" call. The additional specifications are ignored.
But, yes thanks for pointing it out!, because in general that is an invalid input. In the scenario where the user would like to add specifications for both calls "opt" and "freq", then it should be explicitly written as shown in the documentation. I will rise an exception here so that the user knows such input for the specs are wrong. Let me know if you agree or have suggestions.
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, raising an exception would be a good way of handling it. As long as the specifications aren't being ignored silently, it's reasonable.
- Raise exception for not correct specifications for the case of combined calculations (optfreq) input generation. - Added tests
Thanks, that looks good now. There are a couple typos, but nothing problematic. |
Additional structure file
Added h2o.xyz file for tests
Added tests