Skip to content
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

Merged
merged 26 commits into from
Jul 31, 2024
Merged

Mrcc recipes #18

merged 26 commits into from
Jul 31, 2024

Conversation

benshi97
Copy link
Owner

Summary of Changes

Static job recipes for MRCC

@Andrew-S-Rosen
Copy link
Collaborator

Andrew-S-Rosen commented Jul 30, 2024

@benshi97:

with zopen(zpath(Path(pun_file))) as f:

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.

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.58%. Comparing base (760b52c) to head (13dfb5c).
Report is 56 commits behind head on skzcam.

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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Andrew-S-Rosen Andrew-S-Rosen left a 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.

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.

src/quacc/recipes/mrcc/core.py Outdated Show resolved Hide resolved
tests/core/recipes/mrcc_recipes/test_mrcc_recipes.py Outdated Show resolved Hide resolved
tests/core/recipes/mrcc_recipes/test_mrcc_recipes.py Outdated Show resolved Hide resolved
tests/core/recipes/mrcc_recipes/test_mrcc_recipes.py Outdated Show resolved Hide resolved
src/quacc/recipes/mrcc/core.py Outdated Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Collaborator

Andrew-S-Rosen commented Jul 31, 2024


Maybe not needed if we refactor:

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.

I think we should clarify the **kwargs docstring so that the user of MRCC knows what can/can't be taken. In other words, we should mention that it takes charge, mult, mrccinput, mrccblocks and then describe what the difference is between mrccinput and mrccblocks, including a minimal example like in ASE's ORCA class.

For the **kwargs in the function signature, I think we can also type hint that then as **kwargs: MRCCParamsInfo.


Finally, the following type hints don't exist:

from quacc.calculators.mrcc.io import EnergyInfo, ParamsInfo

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.

@benshi97 benshi97 merged commit 5063933 into skzcam Jul 31, 2024
16 of 17 checks passed
@benshi97 benshi97 deleted the mrcc_recipes branch July 31, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants