Fix KspacingMetalHandler triggering on runs that don't use `KSPACIN…

…G` (#298)

* rename ScanMetalHandler to KspacingMetalHandler and deprecate the old name

* fix KspacingMetalHandler.check() returning True if KSPACING not set

* add test_check_with_non_kspacing_wf for KspacingMetalHandler

* add comment explaining reason for DENTET warning

* fix ruff FBT003 Boolean positional value

custodian/custodian.py

@@ -508,13 +508,13 @@ def _run_job(self, job_n, job):
                     if v.check():
                         self.run_log[-1]["validator"] = v
                         s = f"Validation failed: {v.__class__.__name__}"
-                        raise ValidationError(s, True, v)
+                        raise ValidationError(s, raises=True, validator=v)
                 if not zero_return_code:
                     if self.terminate_on_nonzero_returncode:
                         self.run_log[-1]["nonzero_return_code"] = True
                         s = f"Job return code is {p.returncode}. Terminating..."
                         logger.info(s)
-                        raise ReturnCodeError(s, True)
+                        raise ReturnCodeError(s, raises=True)
                     warnings.warn("subprocess returned a non-zero return code. Check outputs carefully...")
                 job.postprocess()
                 return
@@ -524,27 +524,27 @@ def _run_job(self, job_n, job):
                 if not x["actions"] and x["handler"].raises_runtime_error:
                     self.run_log[-1]["handler"] = x["handler"]
                     s = f"Unrecoverable error for handler: {x['handler']}"
-                    raise NonRecoverableError(s, True, x["handler"])
+                    raise NonRecoverableError(s, raises=True, handler=x["handler"])
             for x in self.run_log[-1]["corrections"]:
                 if not x["actions"]:
                     self.run_log[-1]["handler"] = x["handler"]
                     s = f"Unrecoverable error for handler: {x['handler']}"
-                    raise NonRecoverableError(s, False, x["handler"])
+                    raise NonRecoverableError(s, raises=False, handler=x["handler"])
 
         if self.errors_current_job >= self.max_errors_per_job:
             self.run_log[-1]["max_errors_per_job"] = True
             msg = f"Max errors per job reached: {self.max_errors_per_job}."
             logger.info(msg)
-            raise MaxCorrectionsPerJobError(msg, True, self.max_errors_per_job, job)
+            raise MaxCorrectionsPerJobError(msg, raises=True, max_errors_per_job=self.max_errors_per_job, job=job)
 
         self.run_log[-1]["max_errors"] = True
         msg = f"Max errors reached: {self.max_errors}."
         logger.info(msg)
-        raise MaxCorrectionsError(msg, True, self.max_errors)
+        raise MaxCorrectionsError(msg, raises=True, max_errors=self.max_errors)
 
     def run_interrupted(self):
         """
-        Runs custodian in a interuppted mode, which sets up and
+        Runs custodian in a interrupted mode, which sets up and
         validates jobs but doesn't run the executable.
 
         Returns:
@@ -596,7 +596,7 @@ def run_interrupted(self):
                     if not x["actions"] and x["handler"].raises_runtime_error:
                         self.run_log[-1]["handler"] = x["handler"]
                         s = f"Unrecoverable error for handler: {x['handler']}. Raising RuntimeError"
-                        raise NonRecoverableError(s, True, x["handler"])
+                        raise NonRecoverableError(s, raises=True, handler=x["handler"])
                 logger.info("Corrected input based on error handlers")
                 # Return with more jobs to run if recoverable error caught
                 # and corrected for
@@ -609,7 +609,7 @@ def run_interrupted(self):
                     self.run_log[-1]["validator"] = v
                     logger.info("Failed validation based on validator")
                     s = f"Validation failed: {v}"
-                    raise ValidationError(s, True, v)
+                    raise ValidationError(s, raises=True, validator=v)
 
             logger.info(f"Postprocessing for {job.name}.run")
             job.postprocess()
@@ -655,7 +655,9 @@ def _do_check(self, handlers, terminate_func=None):
                         if h.raise_on_max:
                             self.run_log[-1]["handler"] = h
                             self.run_log[-1]["max_errors_per_handler"] = True
-                            raise MaxCorrectionsPerHandlerError(msg, True, h.max_num_corrections, h)
+                            raise MaxCorrectionsPerHandlerError(
+                                msg, raises=True, max_errors_per_handler=h.max_num_corrections, handler=h
+                            )
                         logger.warning(msg + " Correction not applied.")
                         continue
                     if terminate_func is not None and h.is_terminating:
@@ -914,7 +916,7 @@ def __init__(self, message, raises, max_errors_per_job, job):
 class MaxCorrectionsPerHandlerError(CustodianError):
     """Error raised when the maximum allowed number of errors per handler is reached."""
 
-    def __init__(self, message, raises, max_errors_per_handler, handler):
+    def __init__(self, message: str, raises: bool, max_errors_per_handler: int, handler: ErrorHandler) -> None:
         """
         Args:
             message (str): Message passed to Exception

custodian/vasp/handlers.py

@@ -78,7 +78,9 @@ class VaspErrorHandler(ErrorHandler):
         "incorrect_shift": ["Could not get correct shifts"],
         "real_optlay": ["REAL_OPTLAY: internal error", "REAL_OPT: internal ERROR"],
         "rspher": ["ERROR RSPHER"],
-        "dentet": ["DENTET"],
+        "dentet": ["DENTET"],  # reason for this warning is that the Fermi level cannot be determined accurately
+        # enough by the tetrahedron method
+        # https://vasp.at/forum/viewtopic.php?f=3&t=416&p=4047&hilit=dentet#p4047
         "too_few_bands": ["TOO FEW BANDS"],
         "triple_product": ["ERROR: the triple product of the basis vectors"],
         "rot_matrix": ["Found some non-integer element in rotation matrix", "SGRCON"],
@@ -133,7 +135,7 @@ def __init__(
                 lines:
 
                 ```
-                subset = list(VaspErrorHandler().error_msgs.keys())
+                subset = list(VaspErrorHandler().error_msgs)
                 subset.remove("eddrmm")
 
                 handler = VaspErrorHandler(errors_subset_to_catch=subset)
@@ -145,7 +147,7 @@ def __init__(
         self.output_filename = output_filename
         self.errors = set()
         self.error_count = Counter()
-        self.errors_subset_to_catch = errors_subset_to_catch or list(VaspErrorHandler.error_msgs.keys())
+        self.errors_subset_to_catch = errors_subset_to_catch or list(VaspErrorHandler.error_msgs)
         self.vtst_fixes = vtst_fixes
         self.logger = logging.getLogger(self.__class__.__name__)
 
@@ -1158,12 +1160,13 @@ def correct(self):
         return {"errors": ["IncorrectSmearing"], "actions": actions}
 
 
-class ScanMetalHandler(ErrorHandler):
+class KspacingMetalHandler(ErrorHandler):
     """
     Check if a SCAN calculation is a metal (zero bandgap) but has been run with
     a KSPACING value appropriate for semiconductors. If this occurs, this handler
     will rerun the calculation using the KSPACING setting appropriate for metals
-    (KSPACING=0.22). Note that this handler depends on values set in MPScanRelaxSet.
+    (KSPACING=0.22). Note that this handler depends on values set by set_kspacing
+    logic in MPScanRelaxSet.
     """
 
     is_monitor = False
@@ -1182,8 +1185,9 @@ def check(self):
         """Check for error."""
         try:
             v = Vasprun(self.output_filename)
-            # check whether bandgap is zero and tetrahedron smearing was used
-            if v.eigenvalue_band_properties[0] == 0 and v.incar.get("KSPACING", 1) > 0.22:
+            # check whether bandgap is zero and KSPACING is too large
+            # using 0 as fallback value for KSPACING so that this handler does not trigger if KSPACING is not set
+            if v.eigenvalue_band_properties[0] == 0 and v.incar.get("KSPACING", 0) > 0.22:
                 return True
         except Exception:
             pass
@@ -1208,6 +1212,20 @@ def correct(self):
         return {"errors": ["ScanMetal"], "actions": actions}
 
 
+class ScanMetalHandler(KspacingMetalHandler):
+    """ScanMetalHandler was renamed because MP GGA workflow might also adopt kspacing
+    in the future. Keeping this alias during a deprecation period for backwards compatibility.
+    """
+
+    def __init__(self, *args, **kwargs) -> None:
+        warnings.warn(
+            "ScanMetalHandler is deprecated and will be removed in a future release. "
+            "Use KspacingMetalHandler instead.",
+            DeprecationWarning,
+        )
+        super().__init__(*args, **kwargs)
+
+
 class LargeSigmaHandler(ErrorHandler):
     """
     When ISMEAR > 0 (Methfessel-Paxton), monitor the magnitude of the entropy

custodian/vasp/jobs.py

@@ -575,8 +575,8 @@ def constrained_opt_run(
                     x *= 1 + initial_strain
                 else:
                     # Sort the lattice parameter by energies.
-                    min_x = min(energies.keys(), key=lambda e: energies[e])
-                    sorted_x = sorted(energies.keys())
+                    min_x = min(energies, key=lambda e: energies[e])
+                    sorted_x = sorted(energies)
                     ind = sorted_x.index(min_x)
                     if ind == 0:
                         other = ind + 1
@@ -607,7 +607,7 @@ def constrained_opt_run(
                             try:
                                 # If there are more than 4 data points, we will
                                 # do a quadratic fit to accelerate convergence.
-                                x1 = list(energies.keys())
+                                x1 = list(energies)
                                 y1 = [energies[j] for j in x1]
                                 z1 = np.polyfit(x1, y1, 2)
                                 pp = np.poly1d(z1)
@@ -651,10 +651,10 @@ def constrained_opt_run(
                 **vasp_job_kwargs,
             )
 
-        with open("EOS.txt", "w") as f:
-            f.write(f"# {lattice_direction} energy\n")
-            for k in sorted(energies.keys()):
-                f.write(f"{k} {energies[k]}\n")
+        with open("EOS.txt", "w") as file:
+            file.write(f"# {lattice_direction} energy\n")
+            for key in sorted(energies):
+                file.write(f"{key} {energies[key]}\n")
 
     def terminate(self):
         """

custodian/vasp/tests/test_handlers.py

@@ -17,12 +17,14 @@
 
 import pytest
 from pymatgen.io.vasp.inputs import Incar, Kpoints, Structure, VaspInput
+from pymatgen.util.testing import PymatgenTest
 
 from custodian.vasp.handlers import (
     AliasingErrorHandler,
     DriftErrorHandler,
     FrozenJobErrorHandler,
     IncorrectSmearingHandler,
+    KspacingMetalHandler,
     LargeSigmaHandler,
     LrfCommutatorHandler,
     MeshSymmetryErrorHandler,
@@ -809,7 +811,7 @@ def tearDown(self):
         os.chdir(cwd)
 
 
-class ScanMetalHandlerTest(unittest.TestCase):
+class KspacingMetalHandlerTest(PymatgenTest):
     def setUp(self):
         os.environ.setdefault("PMG_VASP_PSP_DIR", test_dir)
         os.chdir(test_dir)
@@ -820,13 +822,24 @@ def setUp(self):
         shutil.copy("vasprun.xml", "vasprun.xml.orig")
 
     def test_check_correct_scan_metal(self):
-        h = ScanMetalHandler()
+        h = KspacingMetalHandler()
         assert h.check()
         d = h.correct()
         assert d["errors"] == ["ScanMetal"]
         assert Incar.from_file("INCAR")["KSPACING"] == 0.22
         os.remove("vasprun.xml")
 
+    def test_check_with_non_kspacing_wf(self):
+        os.chdir(test_dir)
+        shutil.copy("INCAR", f"{self.tmp_path}/INCAR")
+        shutil.copy("vasprun.xml", f"{self.tmp_path}/vasprun.xml")
+        h = KspacingMetalHandler(output_filename=f"{self.tmp_path}/vasprun.xml")
+        assert h.check() is False
+        os.chdir(os.path.join(test_dir, "scan_metal"))
+
+        # TODO (@janosh 2023-11-03) remove when ending ScanMetalHandler deprecation period
+        assert issubclass(ScanMetalHandler, KspacingMetalHandler)
+
     def tearDown(self):
         shutil.move("INCAR.orig", "INCAR")
         shutil.move("vasprun.xml.orig", "vasprun.xml")

test_files/INCAR

@@ -21,4 +21,4 @@ NELMIN = 6
 NPAR = 1
 NSW = 99
 SIGMA = 0.05
-SYMPREC = 1e-5
+SYMPREC = 1e-5