Repository: materialsproject/custodian
Pull request #298 (2023-11-03 20:31:57)

Fix `KspacingMetalHandler` triggering on runs that don't use `KSPACING`
@esoteric-ephemera and I noticed that the `KspacingMetalHandler` (formerly `ScanMetalHandler`) triggers even for runs that don't use `KSPACING` due to using a bad fallback value in

```
v.incar.get("KSPACING", 1) > 0.22
```

https://github.com/materialsproject/custodian/blob/6019d60bf5139fc0559497eb33868a72f00d304d/custodian/vasp/handlers.py#L1185-L1195

If `"KSPACING"` is not in `v.incar` this check will still return `True` and apply the corresponding `KSPACING` reduction correction.

@rkingsbury Would be great to get your feedback on this in case we overlooked something.

The reason for renaming to `KspacingMetalHandler` (`ScanMetalHandler` is still around but deprecated) is that `KSPACING` is not specific to SCAN and in fact may in the future be used by the MP GGA workflows as well.



We added a test in https://github.com/materialsproject/custodian/commit/6019d60bf5139fc0559497eb33868a72f00d304d to make sure the handler does not trigger on an INCAR that doesn't specify `KSPACING`.

Pinging @Andrew-S-Rosen, @utf, @JaGeo in case you'd like to chime in.

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/materialsproject/custodian/pull/298?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) Report
Attention: `9 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`377454c`)](https://app.codecov.io/gh/materialsproject/custodian/commit/377454cfb8aa93f3c8f1af9c5b33123acc80c6f0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) 65.83% compared to head [(`129882c`)](https://app.codecov.io/gh/materialsproject/custodian/pull/298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) 65.92%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
+ Coverage   65.83%   65.92%   +0.09%     
==========================================
  Files          51       51              
  Lines        5628     5641      +13     
==========================================
+ Hits         3705     3719      +14     
+ Misses       1923     1922       -1     
```


| [Files](https://app.codecov.io/gh/materialsproject/custodian/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) | Coverage Δ | |
|---|---|---|
| [custodian/vasp/tests/test\_handlers.py](https://app.codecov.io/gh/materialsproject/custodian/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-Y3VzdG9kaWFuL3Zhc3AvdGVzdHMvdGVzdF9oYW5kbGVycy5weQ==) | `99.78% <100.00%> (+<0.01%)` | :arrow_up: |
| [custodian/vasp/handlers.py](https://app.codecov.io/gh/materialsproject/custodian/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-Y3VzdG9kaWFuL3Zhc3AvaGFuZGxlcnMucHk=) | `78.43% <71.42%> (+0.23%)` | :arrow_up: |
| [custodian/vasp/jobs.py](https://app.codecov.io/gh/materialsproject/custodian/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-Y3VzdG9kaWFuL3Zhc3Avam9icy5weQ==) | `49.72% <0.00%> (ø)` | |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/materialsproject/custodian/pull/298?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject).


Patch:
diff --git a/custodian/custodian.py b/custodian/custodian.py
index 83b39bc0..ff7f6f96 100644
--- a/custodian/custodian.py
+++ b/custodian/custodian.py
@@ -508,13 +508,13 @@ class Custodian:
                     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 @@ class Custodian:
                 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 @@ class Custodian:
                     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 @@ class Custodian:
                     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 @@ class Custodian:
                         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 @@ class MaxCorrectionsPerJobError(CustodianError):
 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
diff --git a/custodian/vasp/handlers.py b/custodian/vasp/handlers.py
index 854ccd36..0d90b3b0 100644
--- a/custodian/vasp/handlers.py
+++ b/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 @@ class VaspErrorHandler(ErrorHandler):
                 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 @@ class VaspErrorHandler(ErrorHandler):
         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 @@ class IncorrectSmearingHandler(ErrorHandler):
         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 @@ class ScanMetalHandler(ErrorHandler):
         """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 @@ class ScanMetalHandler(ErrorHandler):
         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
diff --git a/custodian/vasp/jobs.py b/custodian/vasp/jobs.py
index 71ed3b62..91be503b 100644
--- a/custodian/vasp/jobs.py
+++ b/custodian/vasp/jobs.py
@@ -575,8 +575,8 @@ class VaspJob(Job):
                     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 @@ class VaspJob(Job):
                             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 @@ class VaspJob(Job):
                 **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):
         """


Test patch:
diff --git a/custodian/vasp/tests/test_handlers.py b/custodian/vasp/tests/test_handlers.py
index 5e9acbba..eaf09dc5 100644
--- a/custodian/vasp/tests/test_handlers.py
+++ b/custodian/vasp/tests/test_handlers.py
@@ -17,12 +17,14 @@ import unittest
 
 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 @@ class IncorrectSmearingHandlerFermiTest(unittest.TestCase):
         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 @@ class ScanMetalHandlerTest(unittest.TestCase):
         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")
diff --git a/test_files/INCAR b/test_files/INCAR
index 3c588c38..ce2120d0 100644
--- a/test_files/INCAR
+++ b/test_files/INCAR
@@ -21,4 +21,4 @@ NELMIN = 6
 NPAR = 1
 NSW = 99
 SIGMA = 0.05
-SYMPREC = 1e-5
\ No newline at end of file
+SYMPREC = 1e-5