-
Notifications
You must be signed in to change notification settings - Fork 0
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
Mrcc recipes #18
Mrcc recipes #18
Conversation
quacc/src/quacc/atoms/skzcam.py Line 1138 in aa3af84
Change this to with zopen(zpath(str(Path(pun_file)))) as f: at least until materialsvirtuallab/monty#704 is merged. This will fix your current CI failure. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## skzcam #18 +/- ##
==========================================
- Coverage 98.62% 98.58% -0.04%
==========================================
Files 88 90 +2
Lines 3997 4096 +99
==========================================
+ Hits 3942 4038 +96
- Misses 55 58 +3 ☔ View full report in Codecov by Sentry. |
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.
@benshi97: Thanks for this nice PR! Just a few very minor comments.
Also, can you remind me of what a typical MRCC input looks like (in plaintext)? Mainly, I want to understand a bit more the difference between an "input" and "block". For instance, why do we have to do
output = static_job(
atoms,
charge=0,
spin_multiplicity=1,
method="SCAN",
basis="def2-SVP",
mrccinput={"calc": "PBE", "basis": "STO-3G"},
mrccblocks="symm=off",
)
instead of
output = static_job(
atoms,
charge=0,
spin_multiplicity=1,
method="SCAN",
basis="def2-SVP",
mrccinput={"calc": "PBE", "basis": "STO-3G", "symm": "off"},
)
What makes "symm" special here?
For context, I'm asking because I want to make sure we don't have an unnecessary "mrccblocks
" keyword argument since the whole block stuff is pretty specific to ORCA. If everything can be specified as a key-value pair, then we really only need to have the MRCC
calculator take a set of **kwargs
that we use as-is without having separate mrccinput
and mrccblocks
parameters.
If we do need both mrccinput
and mrccblocks
, it is worth clarifying the docstring for **kwargs
below because it's unclear to the user that they need to be passing in an mrccinput
and mrccblocks
. Naively, one would think you could do MRCC(calc="PBE")
based on the docstring, which isn't actually possible.
quacc/src/quacc/calculators/mrcc/mrcc.py
Lines 192 to 205 in 483c9ea
def __init__( | |
self, *, profile: MrccProfile = None, directory: str | Path = ".", **kwargs | |
) -> None: | |
""" | |
Construct MRCC-calculator object. | |
Parameters | |
---------- | |
profile: MrccProfile | |
The MRCC profile to use. | |
directory: str | |
The directory in which to run the calculation. | |
**kwargs | |
The parameters for the MRCC calculation. |
Let's sort this one out over Zoom.
Maybe not needed if we refactor: quacc/src/quacc/calculators/mrcc/mrcc.py Lines 192 to 205 in 13dfb5c
I think we should clarify the For the Finally, the following type hints don't exist: quacc/src/quacc/calculators/mrcc/mrcc.py Line 22 in 13dfb5c
I'm pretty sure it should be from quacc.types import MRCCEnergyInfo, MRCCParamsInfo and those hints should be used in the rest of the module. |
Summary of Changes
Static job recipes for MRCC