Remove auto_nbands handling + fix bravais inconsistency handling (#409)

* remove auto_nbands handling, throw only a warning

* fix bravais to reflect vasp advice

* typo in auto_nbands fix

* review comments

* try 2

* ruff

* try pytest xdist

* fix lobster test

* give up on pytest xdist

---------

Co-authored-by: Andrew S. Rosen <[email protected]>

Author: esoteric-ephemera

pyproject.toml

@@ -116,7 +116,6 @@ ignore = [
     "D212",    # Multi-line docstring summary should start at the first line
     "ISC001",
     "PD011",   # pandas-use-of-dot-values
-    "PD901",   # pandas-df-variable-name
     "PERF203", # try-except-in-loop
     "PLC0415", # import-outside-top-level (used for performance/optional deps)
     "PLR",     # pylint refactor
@@ -183,6 +182,7 @@ dev = [
     "pymatgen>=2025.5.16",
     "pytest-cov>=6.0.0",
     "pytest>=8.3.5",
+    "pytest-xdist",
     "ruff>=0.11.2",
     "sphinx-markdown-builder>=0.6.8",
     "sphinx>=8.1.3",

src/custodian/lobster/jobs.py

@@ -109,16 +109,12 @@ def run(self, directory="./"):
     def postprocess(self, directory="./") -> None:
         """Will gzip relevant files (won't gzip custodian.json and other output files from the cluster)."""
         if self.gzipped:
-            for file in LOBSTEROUTPUT_FILES:
-                if os.path.isfile(os.path.join(directory, file)):
-                    compress_file(os.path.join(directory, file), compression="gz")
-            for file in LOBSTERINPUT_FILES:
-                if os.path.isfile(os.path.join(directory, file)):
-                    compress_file(os.path.join(directory, file), compression="gz")
-            if self.backup and os.path.isfile(os.path.join(directory, "lobsterin.orig")):
-                compress_file(os.path.join(directory, "lobsterin.orig"), compression="gz")
-            for file in FW_FILES:
-                if os.path.isfile(os.path.join(directory, file)):
-                    compress_file(os.path.join(directory, file), compression="gz")
-            for file in self.add_files_to_gzip:
-                compress_file(os.path.join(directory, file), compression="gz")
+            files_to_zip = (
+                set(LOBSTEROUTPUT_FILES).union(LOBSTERINPUT_FILES).union(FW_FILES).union(self.add_files_to_gzip)
+            )
+            if self.backup:
+                files_to_zip.add("lobsterin.orig")
+
+            for file in files_to_zip:
+                if os.path.isfile(file_path := os.path.join(directory, file)):
+                    compress_file(file_path, compression="gz")

src/custodian/utils.py

@@ -1,11 +1,16 @@
 """Utility function and classes."""
 
+from __future__ import annotations
+
 import functools
 import logging
 import os
 import tarfile
 from glob import glob
-from typing import ClassVar
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from typing import ClassVar
 
 
 def backup(filenames, prefix="error", directory="./") -> None:

src/custodian/vasp/handlers.py

@@ -197,6 +197,19 @@ def check(self, directory="./"):
                         # e-density (brmix error)
                         if err == "brmix" and "NELECT" in incar:
                             continue
+
+                        # Treat auto_nbands only as a warning, do not fail a job
+                        if err == "auto_nbands":
+                            if nbands := self._get_nbands_from_outcar(directory):
+                                outcar = load_outcar(os.path.join(directory, "OUTCAR"))
+                                if (nelect := outcar.nelect) and (nbands > 2 * nelect):
+                                    warnings.warn(
+                                        "NBANDS seems to be too high. The electronic structure may be inaccurate. "
+                                        "You may want to rerun this job with a smaller number of cores.",
+                                        UserWarning,
+                                    )
+                            continue
+
                         self.errors.add(err)
                         error_msgs.add(msg)
         for msg in error_msgs:
@@ -538,12 +551,11 @@ def correct(self, directory="./"):
 
             self.error_count["zbrent"] += 1
 
-        if "too_few_bands" in self.errors:
-            nbands = None
-            nbands = vi["INCAR"]["NBANDS"] if "NBANDS" in vi["INCAR"] else self._get_nbands_from_outcar(directory)
-            if nbands:
-                new_nbands = max(int(1.1 * nbands), nbands + 1)  # This handles the case when nbands is too low (< 8).
-                actions.append({"dict": "INCAR", "action": {"_set": {"NBANDS": new_nbands}}})
+        if "too_few_bands" in self.errors and (
+            nbands := vi["INCAR"].get("NBANDS") or self._get_nbands_from_outcar(directory)
+        ):
+            new_nbands = max(int(1.1 * nbands), nbands + 1)  # This handles the case when nbands is too low (< 8).
+            actions.append({"dict": "INCAR", "action": {"_set": {"NBANDS": new_nbands}}})
 
         if self.errors & {"pssyevx", "pdsyevx"} and vi["INCAR"].get("ALGO", "Normal").lower() != "normal":
             actions.append({"dict": "INCAR", "action": {"_set": {"ALGO": "Normal"}}})
@@ -667,23 +679,20 @@ def correct(self, directory="./"):
 
         if self.errors.intersection(["bravais", "ksymm"]):
             # For bravais: VASP recommends refining the lattice parameters
-            # or changing SYMPREC. See https://www.vasp.at/forum/viewtopic.php?f=3&t=19109
+            # or changing SYMPREC (default = 1e-5). See
+            # https://www.vasp.at/forum/viewtopic.php?f=3&t=19109
             # Appears to occur when SYMPREC is very low, so we change it to
             # the default if it's not already. If it's the default, we x10.
             # For ksymm, there's not much information about the issue other than the
             # direct and reciprocal meshes being incompatible.
             # This is basically the same as bravais
             vasp_recommended_symprec = 1e-6
-            symprec = vi["INCAR"].get("SYMPREC", vasp_recommended_symprec)
-            if symprec < vasp_recommended_symprec:
-                actions.append({"dict": "INCAR", "action": {"_set": {"SYMPREC": vasp_recommended_symprec}}})
-            elif symprec < 1e-4:
-                # try 10xing symprec twice, then set ISYM=0 to not impose potentially artificial symmetry from
-                # too loose symprec on charge density
-                actions.append({"dict": "INCAR", "action": {"_set": {"SYMPREC": float(f"{symprec * 10:.1e}")}}})
-            else:
+            if (symprec := vi["INCAR"].get("SYMPREC", 1e-5)) > vasp_recommended_symprec:
+                actions.append(
+                    {"dict": "INCAR", "action": {"_set": {"SYMPREC": min(symprec / 10.0, vasp_recommended_symprec)}}}
+                )
+            elif vi["INCAR"].get("ISYM") > 0:  # Default ISYM is variable, but never 0
                 actions.append({"dict": "INCAR", "action": {"_set": {"ISYM": 0}}})
-            self.error_count["bravais"] += 1
 
         if "nbands_not_sufficient" in self.errors:
             outcar = load_outcar(os.path.join(directory, "OUTCAR"))
@@ -749,50 +758,25 @@ def correct(self, directory="./"):
                     )
             self.error_count["algo_tet"] += 1
 
-        if "auto_nbands" in self.errors and (nbands := self._get_nbands_from_outcar(directory)):
-            outcar = load_outcar(os.path.join(directory, "OUTCAR"))
-
-            if (nelect := outcar.nelect) and (nbands > 2 * nelect):
-                self.error_count["auto_nbands"] += 1
-                warnings.warn(
-                    "NBANDS seems to be too high. The electronic structure may be inaccurate. "
-                    "You may want to rerun this job with a smaller number of cores.",
-                    UserWarning,
-                )
-
-            elif nbands := vi["INCAR"].get("NBANDS"):
-                kpar = vi["INCAR"].get("KPAR", 1)
-                ncore = vi["INCAR"].get("NCORE", 1)
-                # If the user set an NBANDS that isn't compatible with parallelization settings,
-                # increase NBANDS to ensure correct task distribution and issue a UserWarning.
-                # The number of ranks per band is (number of MPI ranks) / (KPAR * NCORE)
-                if (ranks := outcar.run_stats.get("cores")) and (rem_bands := nbands % (ranks // (kpar * ncore))) != 0:
-                    actions.append({"dict": "INCAR", "action": {"_set": {"NBANDS": nbands + rem_bands}}})
-                    warnings.warn(
-                        f"Your NBANDS={nbands} setting was incompatible with your parallelization "
-                        f"settings, KPAR={kpar}, NCORE={ncore}, over {ranks} ranks. "
-                        f"The number of bands has been decreased accordingly to {nbands + rem_bands}.",
-                        UserWarning,
-                    )
-
         VaspModder(vi=vi, directory=directory).apply_actions(actions)
         return {"errors": list(self.errors), "actions": actions}
 
     @staticmethod
     def _get_nbands_from_outcar(directory: str) -> int | None:
         nbands = None
-        with open(os.path.join(directory, "OUTCAR")) as file:
-            for line in file:
-                # Have to take the last NBANDS line since sometimes VASP
-                # updates it automatically even if the user specifies it.
-                # The last one is marked by NBANDS= (no space).
-                if "NBANDS=" in line:
-                    try:
-                        d = line.split("=")
-                        nbands = int(d[-1].strip())
-                        break
-                    except (IndexError, ValueError):
-                        pass
+        if os.path.isfile(outcar_path := os.path.join(directory, "OUTCAR")):
+            with open(outcar_path) as file:
+                for line in file:
+                    # Have to take the last NBANDS line since sometimes VASP
+                    # updates it automatically even if the user specifies it.
+                    # The last one is marked by NBANDS= (no space).
+                    if "NBANDS=" in line:
+                        try:
+                            d = line.split("=")
+                            nbands = int(d[-1].strip())
+                            break
+                        except (IndexError, ValueError):
+                            pass
         return nbands
 
 

tests/lobster/test_jobs.py

@@ -1,5 +1,7 @@
 import os
 import shutil
+from pathlib import Path
+from tempfile import TemporaryDirectory
 
 from monty.os import cd
 from monty.tempfile import ScratchDir
@@ -82,36 +84,38 @@ def test_setup(self) -> None:
 
     def test_postprocess(self) -> None:
         # test gzipped and zipping of additional files
-        with cd(os.path.join(test_files_lobster3)):
-            with ScratchDir(".", copy_from_current_on_enter=True):
-                shutil.copy("lobsterin", "lobsterin.orig")
-                v = LobsterJob("hello", gzipped=True, add_files_to_gzip=VASP_OUTPUT_FILES)
-                v.postprocess()
-                for file in (*VASP_OUTPUT_FILES, *LOBSTER_FILES, *FW_FILES):
-                    assert os.path.isfile(f"{file}.gz")
-
-            with ScratchDir(".", copy_from_current_on_enter=True):
-                shutil.copy("lobsterin", "lobsterin.orig")
-                v = LobsterJob("hello", gzipped=False, add_files_to_gzip=VASP_OUTPUT_FILES)
-                v.postprocess()
-                for file in (*VASP_OUTPUT_FILES, *LOBSTER_FILES, *FW_FILES):
-                    assert os.path.isfile(file)
+        src_path = Path(test_files_lobster3).absolute()
+        for gzipped in (True, False):
+            with TemporaryDirectory() as tmp_dir:
+                cwd = Path(tmp_dir).absolute()
+                for file_obj in Path(src_path).glob("*"):
+                    if file_obj.is_file():
+                        shutil.copy(src_path / file_obj, cwd / file_obj.name)
+                    shutil.copy(src_path / "lobsterin", cwd / "lobsterin.orig")
+                with cd(cwd):
+                    v = LobsterJob("hello", gzipped=gzipped, add_files_to_gzip=VASP_OUTPUT_FILES)
+                    v.postprocess()
+                    assert all(
+                        os.path.isfile(f"{file}{'.gz' if gzipped else ''}")
+                        for file in (*VASP_OUTPUT_FILES, *LOBSTER_FILES, *FW_FILES)
+                    )
 
     def test_postprocess_v51(self) -> None:
         # test gzipped and zipping of additional files for lobster v5.1
-        with cd(os.path.join(test_files_lobster4)):
-            with ScratchDir(".", copy_from_current_on_enter=True):
-                shutil.copy("lobsterin", "lobsterin.orig")
-                v = LobsterJob("hello", gzipped=True, add_files_to_gzip=VASP_OUTPUT_FILES)
-                v.postprocess()
-                for file in (*VASP_OUTPUT_FILES, *LOBSTEROUTPUT_FILES, *FW_FILES):
-                    if file not in ("POSCAR.lobster", "bandOverlaps.lobster"):  # these files are not in the directory
-                        assert os.path.isfile(f"{file}.gz")
-
-            with ScratchDir(".", copy_from_current_on_enter=True):
-                shutil.copy("lobsterin", "lobsterin.orig")
-                v = LobsterJob("hello", gzipped=False, add_files_to_gzip=VASP_OUTPUT_FILES)
-                v.postprocess()
-                for file in (*VASP_OUTPUT_FILES, *LOBSTEROUTPUT_FILES, *FW_FILES):
-                    if file not in ("POSCAR.lobster", "bandOverlaps.lobster"):  # these files are not in the directory
-                        assert os.path.isfile(file)
+        src_path = Path(test_files_lobster4).absolute()
+        for gzipped in (True, False):
+            with TemporaryDirectory() as tmp_dir:
+                cwd = Path(tmp_dir).absolute()
+                for file_obj in Path(src_path).glob("*"):
+                    if file_obj.is_file():
+                        shutil.copy(src_path / file_obj, cwd / file_obj.name)
+                    shutil.copy(src_path / "lobsterin", cwd / "lobsterin.orig")
+                with cd(cwd):
+                    v = LobsterJob("hello", gzipped=gzipped, add_files_to_gzip=VASP_OUTPUT_FILES)
+                    v.postprocess()
+                    assert all(
+                        os.path.isfile(f"{file}{'.gz' if gzipped else ''}")
+                        for file in {*VASP_OUTPUT_FILES, *LOBSTEROUTPUT_FILES, *FW_FILES}.difference(
+                            {"POSCAR.lobster", "bandOverlaps.lobster"}  # these files are not in the directory
+                        )
+                    )