Merge pull request materialsproject#576 from materialsproject/fix-_se…

…t_kspacing

Fix `VaspInputGenerator`'s `_set_kspacing` not respecting `auto_ismear = False` nor `auto_kspacing = False`

.pre-commit-config.yaml

@@ -3,20 +3,20 @@ default_language_version:
 exclude: '^.github/'
 repos:
 - repo: https://github.com/charliermarsh/ruff-pre-commit
-  rev: v0.0.287
+  rev: v0.1.0
   hooks:
   - id: ruff
     args: [--fix]
 - repo: https://github.com/pre-commit/pre-commit-hooks
-  rev: v4.4.0
+  rev: v4.5.0
   hooks:
   - id: check-yaml
   - id: fix-encoding-pragma
     args: [--remove]
   - id: end-of-file-fixer
   - id: trailing-whitespace
 - repo: https://github.com/psf/black
-  rev: 23.7.0
+  rev: 23.10.0
   hooks:
   - id: black
 - repo: https://github.com/asottile/blacken-docs
@@ -46,7 +46,7 @@ repos:
   - id: rst-directive-colons
   - id: rst-inline-touching-normal
 - repo: https://github.com/pre-commit/mirrors-mypy
-  rev: v1.5.1
+  rev: v1.6.1
   hooks:
   - id: mypy
     files: ^src/
@@ -55,9 +55,9 @@ repos:
     - types-pkg_resources==0.1.2
     - types-paramiko
 - repo: https://github.com/codespell-project/codespell
-  rev: v2.2.5
+  rev: v2.2.6
   hooks:
   - id: codespell
     stages: [commit, commit-msg]
-    args: [--ignore-words-list, 'titel,statics,ba,nd,te']
+    args: [--ignore-words-list, 'titel,statics,ba,nd,te,atomate']
     types_or: [python, rst, markdown]

src/atomate2/forcefields/utils.py

@@ -127,9 +127,9 @@ def __init__(
 
         Parameters
         ----------
-        calculator (ase Calculatur): an ase calculator
+        calculator (ase Calculator): an ase calculator
         optimizer (str or ase Optimizer): the optimization algorithm.
-        relax_cell (bool): if True, cell parameters will be opitimized.
+        relax_cell (bool): if True, cell parameters will be optimized.
         """
         self.calculator = calculator
 

src/atomate2/vasp/flows/mp.py

@@ -132,7 +132,7 @@ def make(self, structure: Structure, prev_vasp_dir: str | Path | None = None):
             output = static_job.output
             jobs += [static_job]
 
-        return Flow(jobs, output, name=self.name)
+        return Flow(jobs=jobs, output=output, name=self.name)
 
 
 @dataclass
@@ -193,4 +193,4 @@ def make(self, structure: Structure, prev_vasp_dir: str | Path | None = None):
             output = static_job.output
             jobs += [static_job]
 
-        return Flow(jobs, output=output, name=self.name)
+        return Flow(jobs=jobs, output=output, name=self.name)

src/atomate2/vasp/sets/base.py

@@ -674,27 +674,26 @@ def _get_incar(
             auto_updates["ISPIN"] = ispin
 
         if self.auto_ismear:
+            bandgap_tol = getattr(self, "bandgap_tol", SETTINGS.BANDGAP_TOL)
             if bandgap is None:
                 # don't know if we are a metal or insulator so set ISMEAR and SIGMA to
                 # be safe with the most general settings
                 auto_updates.update(ISMEAR=0, SIGMA=0.2)
-            elif bandgap == 0:
+            elif bandgap <= bandgap_tol:
                 auto_updates.update(ISMEAR=2, SIGMA=0.2)  # metal
             else:
                 auto_updates.update(ISMEAR=-5, SIGMA=0.05)  # insulator
 
         if self.auto_lreal:
             auto_updates["LREAL"] = _get_recommended_lreal(structure)
 
-        _set_kspacing(
-            incar,
-            incar_settings,
-            self.user_incar_settings,
-            self.auto_kspacing if isinstance(self.auto_kspacing, float) else bandgap,
-            kpoints,
-            previous_incar is None,
-            bandgap_tol=getattr(self, "bandgap_tol", SETTINGS.BANDGAP_TOL),
-        )
+        if self.auto_kspacing is False:
+            bandgap = None  # don't auto-set KSPACING based on bandgap
+        elif isinstance(self.auto_kspacing, float):
+            # interpret auto_kspacing as bandgap and set KSPACING based on user input
+            bandgap = self.auto_kspacing
+        _set_kspacing(incar, incar_settings, self.user_incar_settings, bandgap, kpoints)
+
         # apply updates from auto options, careful not to override user_incar_settings
         _apply_incar_updates(incar, auto_updates, skip=list(self.user_incar_settings))
 
@@ -1087,10 +1086,8 @@ def _set_kspacing(
     incar: Incar,
     incar_settings: dict,
     user_incar_settings: dict,
-    bandgap: float,
-    kpoints: Kpoints,
-    from_prev: bool,
-    bandgap_tol: float,
+    bandgap: float | None,
+    kpoints: Kpoints | None,
 ):
     """
     Set KSPACING in an INCAR.
@@ -1115,21 +1112,14 @@ def _set_kspacing(
 
     elif "KSPACING" in user_incar_settings:
         incar["KSPACING"] = user_incar_settings["KSPACING"]
-    elif incar_settings.get("KSPACING") and bandgap:
+    elif incar_settings.get("KSPACING") and isinstance(bandgap, float):
         # will always default to 0.22 in first run as one
         # cannot be sure if one treats a metal or
         # semiconductor/insulator
         incar["KSPACING"] = _get_kspacing(bandgap)
         # This should default to ISMEAR=0 if band gap is not known (first computation)
         # if not from_prev:
         #     # be careful to not override user_incar_settings
-        if not from_prev:
-            if bandgap <= bandgap_tol:
-                incar["SIGMA"] = user_incar_settings.get("SIGMA", 0.2)
-                incar["ISMEAR"] = user_incar_settings.get("ISMEAR", 2)
-            else:
-                incar["SIGMA"] = user_incar_settings.get("SIGMA", 0.05)
-                incar["ISMEAR"] = user_incar_settings.get("ISMEAR", -5)
     elif incar_settings.get("KSPACING"):
         incar["KSPACING"] = incar_settings["KSPACING"]
 

src/atomate2/vasp/sets/matpes.py

@@ -12,7 +12,7 @@
 from monty.serialization import loadfn
 from pkg_resources import resource_filename
 
-from atomate2.vasp.sets.core import StaticSetGenerator
+from atomate2.vasp.sets.base import VaspInputGenerator
 
 if TYPE_CHECKING:
     from pymatgen.core import Structure
@@ -31,30 +31,49 @@
 
 
 @dataclass
-class MatPesGGAStaticSetGenerator(StaticSetGenerator):
+class MatPesGGAStaticSetGenerator(VaspInputGenerator):
     """Class to generate MP-compatible VASP GGA static input sets."""
 
     config_dict: dict = field(default_factory=lambda: _BASE_MATPES_PBE_STATIC_SET)
     auto_ismear: bool = False
-    auto_kspacing: bool = True
-    user_incar_settings: dict = field(
-        # ensure _set_kspacing doesn't override input set ISMEAR
-        default_factory=lambda: {"ISMEAR": 0, "SIGMA": 0.05}
-    )
+    auto_kspacing: bool = False
+
+    def get_incar_updates(
+        self,
+        structure: Structure,
+        prev_incar: dict = None,
+        bandgap: float = None,
+        vasprun: Vasprun = None,
+        outcar: Outcar = None,
+    ) -> dict:
+        """
+        Get updates to the INCAR for this calculation type.
+
+        Parameters
+        ----------
+        structure
+            A structure.
+        prev_incar
+            An incar from a previous calculation.
+        bandgap
+            The band gap.
+        vasprun
+            A vasprun from a previous calculation.
+        outcar
+            An outcar from a previous calculation.
+
+        Returns
+        -------
+        dict
+            A dictionary of updates to apply.
+        """
+        return {}
 
 
 @dataclass
-class MatPesMetaGGAStaticSetGenerator(StaticSetGenerator):
+class MatPesMetaGGAStaticSetGenerator(MatPesGGAStaticSetGenerator):
     """Class to generate MP-compatible VASP GGA static input sets."""
 
-    config_dict: dict = field(default_factory=lambda: _BASE_MATPES_PBE_STATIC_SET)
-    auto_ismear: bool = False
-    auto_kspacing: bool = True
-    user_incar_settings: dict = field(
-        # ensure _set_kspacing doesn't override input set ISMEAR
-        default_factory=lambda: {"ISMEAR": 0, "SIGMA": 0.05}
-    )
-
     def get_incar_updates(
         self,
         structure: Structure,

src/atomate2/vasp/sets/mp.py

@@ -33,7 +33,6 @@ class MPGGARelaxSetGenerator(RelaxSetGenerator):
     """Class to generate MP-compatible VASP GGA relaxation input sets."""
 
     config_dict: dict = field(default_factory=lambda: _BASE_MP_GGA_RELAX_SET)
-    auto_ismear: bool = False
     auto_kspacing: bool = True
     inherit_incar: bool = False
 
@@ -90,7 +89,6 @@ class MPMetaGGAStaticSetGenerator(StaticSetGenerator):
     """Class to generate MP-compatible VASP GGA static input sets."""
 
     config_dict: dict = field(default_factory=lambda: _BASE_MP_R2SCAN_RELAX_SET)
-    auto_ismear: bool = False
     auto_kspacing: bool = True
     inherit_incar: bool = False
 
@@ -149,7 +147,6 @@ class MPMetaGGARelaxSetGenerator(RelaxSetGenerator):
 
     config_dict: dict = field(default_factory=lambda: _BASE_MP_R2SCAN_RELAX_SET)
     bandgap_tol: float = 1e-4
-    auto_ismear: bool = False
     auto_kspacing: bool = True
     inherit_incar: bool = False
 

tests/test_data/vasp/Si_mp_gga_relax/GGA_Relax_1/inputs/INCAR

@@ -3,7 +3,7 @@ EDIFF = 0.0001
 ENCUT = 520
 IBRION = 2
 ISIF = 3
-ISMEAR = -5
+ISMEAR = 0
 ISPIN = 2
 LASPH = True
 LORBIT = 11
@@ -14,4 +14,4 @@ MAGMOM = 2*0.6
 NELM = 100
 NSW = 99
 PREC = Accurate
-SIGMA = 0.05
+SIGMA = 0.2

tests/test_data/vasp/Si_mp_meta_gga_relax/pbesol_pre_relax/inputs/INCAR

@@ -22,4 +22,4 @@ MAGMOM = 2*0.6
 NELM = 200
 NSW = 99
 PREC = Accurate
-SIGMA = 0.05
+SIGMA = 0.2

tests/test_data/vasp/matpes_pbe_r2scan_flow/r2scan_static/inputs/INCAR

@@ -4,7 +4,7 @@ ENAUG = 1360.0
 ENCUT = 680
 ISMEAR = 0
 ISPIN = 2
-KSPACING = 0.28754316109307704
+KSPACING = 0.22
 LAECHG = True
 LASPH = True
 LCHARG = True

tests/vasp/conftest.py

@@ -217,7 +217,7 @@ def check_kpoints(ref_path: Path):
         user_ksp, ref_ksp = user_incar.get("KSPACING"), ref_incar.get("KSPACING")
         if user_ksp != ref_ksp:
             raise ValueError(
-                f"\n\nKSPACING is inconsistent: {user_ksp} != {ref_ksp} "
+                f"\n\nKSPACING is inconsistent: expected {ref_ksp}, got {user_ksp} "
                 f"\nin ref file {ref_incar_path}"
             )
 

tests/vasp/flows/test_matpes.py

@@ -8,7 +8,7 @@
 from atomate2.vasp.flows.matpes import MatPesGGAPlusMetaGGAStaticMaker
 
 
-def test_mp_meta_gga_double_relax_static(mock_vasp, clean_dir, vasp_test_dir):
+def test_matpes_gga_plus_meta_gga_static_maker(mock_vasp, clean_dir, vasp_test_dir):
     # map from job name to directory containing reference output files
     pre_relax_dir = "matpes_pbe_r2scan_flow/pbe_static"
     ref_paths = {

tests/vasp/flows/test_mp.py

@@ -67,8 +67,8 @@ def test_mp_meta_gga_double_relax_static(mock_vasp, clean_dir, vasp_test_dir):
     # generate flow
     flow = MPMetaGGADoubleRelaxStaticMaker(
         relax_maker2=MPMetaGGARelaxMaker(
-            # TODO better test for bandgap_tol, isn't actually run by mock_vasp
-            # this just tests it can be passed without error
+            # TODO write better test for bandgap_tol since it isn't actually used by
+            # mock_vasp. this just tests it can be passed without error
             input_set_generator=MPMetaGGARelaxSetGenerator(bandgap_tol=0.1)
         )
     ).make(si_struct)

tests/vasp/jobs/test_matpes.py

@@ -119,11 +119,7 @@ def test_matpes_meta_gga_static_maker(mock_vasp, clean_dir, vasp_test_dir):
     mock_vasp(ref_paths, fake_run_vasp_kwargs)
 
     # generate flow
-    job = MatPesMetaGGAStaticMaker(
-        input_set_generator=MatPesMetaGGAStaticSetGenerator(
-            auto_kspacing=0.6172000000000004
-        )
-    ).make(si_struct)
+    job = MatPesMetaGGAStaticMaker().make(si_struct)
 
     # ensure flow runs successfully
     responses = run_locally(job, create_folders=True, ensure_success=True)