Repository: Quantum-Accelerators/quacc
Pull request #1335 (2023-12-11 17:29:42)

Espresso: managing various outdirs
## Required Checklist

- [x] I have read the [Contributing Guide](https://quantum-accelerators.github.io/quacc/dev/contributing.html). Don't lie! 😉
- [x] My PR is on a custom branch and is _not_ named `main`.
- [x] I have added relevant unit tests. Note: Your PR will likely not be merged without proper tests.

If you are an external contributor, you will see a comment from @buildbot-princeton. You can ignore this, as it is solely for admin purposes.

## Summary of Changes

Edit: No need to be that complicated, this PR just makes sure that chosen outdirs exist if inside the calc_dir.

If the user decides to ask for outdirs that are outside the calc_dir via absolute paths, then there is nothing we should do (?). It is the user responsibility to deal with that data (this PR add a dictionary to keep track of that data but does nothing with it).

buildbot-princeton: Can one of the admins verify this patch?
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1335?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) Report
Attention: `10 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`40c21e6`)](https://app.codecov.io/gh/Quantum-Accelerators/quacc/commit/40c21e6799289911cca0652da5af963cff0a1974?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) 97.77% compared to head [(`0f8b3a2`)](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1335?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) 97.45%.
> Report is 1 commits behind head on main.

| [Files](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1335?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) | Patch % | Lines |
|---|---|---|
| [src/quacc/calculators/espresso/espresso.py](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1335?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators#diff-c3JjL3F1YWNjL2NhbGN1bGF0b3JzL2VzcHJlc3NvL2VzcHJlc3NvLnB5) | 60.00% | [10 Missing :warning: ](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1335?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) |

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


```diff
@@            Coverage Diff             @@
##             main    #1335      +/-   ##
==========================================
- Coverage   97.77%   97.45%   -0.33%     
==========================================
  Files          75       75              
  Lines        2922     2946      +24     
==========================================
+ Hits         2857     2871      +14     
- Misses         65       75      +10     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1335?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators).   
: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=Quantum-Accelerators).

tomdemeyere: > Thanks for adding these sanity checks! I left a few small comments, and we'll also need some more tests to evaluate the behavior of the new functionality, as you know.
> 
> > If the user decides to ask for outdirs that are outside the calc_dir via absolute paths, then there is nothing we should do (?). It is the user responsibility to deal with that data (this PR add a dictionary to keep track of that data but does nothing with it).
> 
> Should we simply forbid setting of those `outdirs` then? Perhaps that's a simpler solution altogether.

I don't think we should forbid it as it will never make anything crash if used properly (with this patch), and it would remove some flexibility. The only cases where it might cause an issue are:

- If the user specify a path outside quacc that does not exist.
- If the user is inconsistent in his choice of outdir in the same flow.

But I would argue that these are cases where the calculation deserves to crash anyway. Also, this gives an alternative to copying files around, for people with a very small scratch space this might be useful. If we keep it, maybe I should add paragraph in the doc to explain all that.
Andrew-S-Rosen: > I don't think we should forbid it as it will never make anything crash if used properly (with this patch), and it would remove some flexibility. The only cases where it might cause an issue are:

I see. That makes sense then. Thanks for clarifying.

>  Also, this gives an alternative to copying files around, for people with a very small scratch space this might be useful. If we keep it, maybe I should add paragraph in the doc to explain all that.

In this scenario, the user could simply not set a `SCRATCH_DIR` setting, and then there will be no file shuttling. If set, then files are moved (but not copied).

Certainly, if desired, additional details can be proved in a docstring, which is rendered in the API documentation.
Andrew-S-Rosen: Thanks for the really great test! To get this one merged, there's just a few more lines that'd be great to cover. To make the process quicker, no need to do a "real" calculation to address the remaining uncovered lines. You can simply add the following to `tests/core/calculators/espresso/test_espresso.py`:

```python
from quacc.calculators.espresso.espresso import EspressoTemplate

def test_espresso_write_input(monkeypatch, tmp_dir):
    monkeypatch.chdir(tmp_dir)
    params = {...}
    new_params = EspressoTemplate._outdir_handler(params)
    # do assertions on `new_params`
```
Andrew-S-Rosen: @tomdemeyere: Is this ready to merge? I made some very minor changes to reflect the open comments. Thanks for this nice PR!

@buildbot-princeton: test this please.
tomdemeyere: Ready to merge

Patch:
diff --git a/src/quacc/calculators/espresso/espresso.py b/src/quacc/calculators/espresso/espresso.py
index 5486b0e2..d87f066d 100644
--- a/src/quacc/calculators/espresso/espresso.py
+++ b/src/quacc/calculators/espresso/espresso.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import os
 from pathlib import Path
 from typing import TYPE_CHECKING
 
@@ -42,10 +43,17 @@ class EspressoTemplate(EspressoTemplate_):
         None
         """
         super().__init__()
+
         self.inputname = f"{binary}.in"
         self.outputname = f"{binary}.out"
+
         self.binary = binary
 
+        self.outdirs = {
+            "outdir": os.environ.get("ESPRESSO_TMPDIR"),
+            "wfcdir": os.environ.get("ESPRESSO_TMPDIR"),
+        }
+
     def write_input(
         self,
         profile: EspressoProfile,
@@ -75,8 +83,11 @@ class EspressoTemplate(EspressoTemplate_):
         -------
         None
         """
+        directory = Path(directory)
+        self._outdir_handler(parameters, directory)
+
         write(
-            Path(directory) / self.inputname,
+            directory / self.inputname,
             atoms,
             binary=self.binary,
             properties=properties,
@@ -101,11 +112,52 @@ class EspressoTemplate(EspressoTemplate_):
         dict
             The results dictionnary
         """
+
         results = read(Path(directory) / self.outputname, binary=self.binary)
         if "energy" not in results:
             results["energy"] = None
         return results
 
+    def _outdir_handler(self, parameters: dict[str, Any], directory: Path) -> dict[str, Any]:
+        """
+        Function that handles the various outdir of espresso binaries. If they are relative,
+        they are resolved against `directory`, which is the recommended approach.
+        If the user-supplied paths are absolute, they are resolved and checked
+        against `directory`, which is typically `os.getcwd()`. If they are not in `directory`,
+        they will be ignored.
+
+        Parameters
+        ----------
+        parameters
+            User-supplied kwargs
+        directory
+            The `directory` kwarg from the calculator.
+
+        Returns
+        -------
+        dict[str, Any]
+            The merged kwargs
+        """
+
+        input_data = parameters.get("input_data", {})
+
+        for section in input_data:
+            for d_key in self.outdirs.keys():
+                if d_key in input_data[section]:
+                    path = Path(input_data[section][d_key])
+                    path = path.expanduser().resolve()
+                    if directory.expanduser().resolve() not in path.parents:
+                        self.outdirs[d_key] = path
+                        continue
+                    path.mkdir(parents=True, exist_ok=True)
+                    input_data[section][d_key] = path
+
+        self.outdirs = [path for path in self.outdirs.values() if path is not None]
+
+        parameters["input_data"] = input_data
+
+        return parameters
+
 
 class Espresso(Espresso_):
     """
@@ -180,7 +232,12 @@ class Espresso(Espresso_):
             binary=str(bin_path), parallel_info=parallel_info, pseudo_path=pseudo_path
         )
 
-        super().__init__(profile=profile, parallel_info=parallel_info, **kwargs)
+        if kwargs.get("directory"):
+            raise ValueError("quacc does not support the directory argument.")
+
+        super().__init__(
+            profile=profile, directory=".", parallel_info=parallel_info, **kwargs
+        )
 
         self.template = template
 
diff --git a/src/quacc/recipes/emt/defects.py b/src/quacc/recipes/emt/defects.py
index 04312c99..31b29d1d 100644
--- a/src/quacc/recipes/emt/defects.py
+++ b/src/quacc/recipes/emt/defects.py
@@ -77,12 +77,12 @@ def bulk_to_defects_flow(
     make_defects_kwargs = merge_dicts(
         make_defects_kwargs, {"defect_gen": defect_gen, "defect_charge": defect_charge}
     )
+    defect_relax_job = relax_job if custom_relax_job is None else custom_relax_job
+    defect_static_job = static_job if custom_static_job is None else custom_static_job
 
     return bulk_to_defects_subflow(
         atoms,
-        relax_job if custom_relax_job is None else custom_relax_job,
-        static_job=(static_job if custom_static_job is None else custom_static_job)
-        if run_static
-        else None,
+        defect_relax_job,
+        static_job=defect_static_job if run_static else None,
         make_defects_kwargs=make_defects_kwargs,
     )
diff --git a/src/quacc/recipes/emt/phonons.py b/src/quacc/recipes/emt/phonons.py
index 4d5b675e..28c79104 100644
--- a/src/quacc/recipes/emt/phonons.py
+++ b/src/quacc/recipes/emt/phonons.py
@@ -54,10 +54,11 @@ def phonon_flow(
     PhononSchema
         Dictionary of results from [quacc.schemas.phonons.summarize_phonopy][]
     """
+    phonon_static_job = static_job if custom_static_job is None else custom_static_job
 
     return phonon_flow_(
         atoms,
-        static_job if custom_static_job is None else custom_static_job,
+        phonon_static_job,
         supercell_matrix=supercell_matrix,
         atom_disp=atom_disp,
         t_step=t_step,
diff --git a/src/quacc/recipes/emt/slabs.py b/src/quacc/recipes/emt/slabs.py
index cb048a93..823a6697 100644
--- a/src/quacc/recipes/emt/slabs.py
+++ b/src/quacc/recipes/emt/slabs.py
@@ -53,12 +53,12 @@ def bulk_to_slabs_flow(
         [RunSchema][quacc.schemas.ase.summarize_run] or
         [OptSchema][quacc.schemas.ase.summarize_opt_run] for each slab.
     """
+    slab_relax_job = relax_job if custom_relax_job is None else custom_relax_job
+    slab_static_job = static_job if custom_static_job is None else custom_static_job
 
     return bulk_to_slabs_subflow(
         atoms,
-        relax_job if custom_relax_job is None else custom_relax_job,
-        static_job=(static_job if custom_static_job is None else custom_relax_job)
-        if run_static
-        else None,
+        slab_relax_job,
+        static_job=slab_static_job if run_static else None,
         make_slabs_kwargs=make_slabs_kwargs,
     )
diff --git a/src/quacc/recipes/vasp/slabs.py b/src/quacc/recipes/vasp/slabs.py
index 412c6ce2..5f4f8fb0 100644
--- a/src/quacc/recipes/vasp/slabs.py
+++ b/src/quacc/recipes/vasp/slabs.py
@@ -152,13 +152,13 @@ def bulk_to_slabs_flow(
     list[VaspSchema]
         List of dictionary results from [quacc.schemas.vasp.vasp_summarize_run][]
     """
+    slab_relax_job = relax_job if custom_relax_job is None else custom_relax_job
+    slab_static_job = static_job if custom_static_job is None else custom_static_job
 
     return bulk_to_slabs_subflow(
         atoms,
-        relax_job if custom_relax_job is None else custom_relax_job,
-        static_job=(static_job if custom_static_job is None else custom_static_job)
-        if run_static
-        else None,
+        slab_relax_job,
+        static_job=slab_static_job if run_static else None,
         make_slabs_kwargs=make_slabs_kwargs,
     )
 
@@ -202,12 +202,13 @@ def slab_to_ads_flow(
         List of dictionaries of results from [quacc.schemas.vasp.vasp_summarize_run][]
     """
 
+    slab_relax_job = relax_job if custom_relax_job is None else custom_relax_job
+    slab_static_job = static_job if custom_static_job is None else custom_static_job
+
     return slab_to_ads_subflow(
         slab,
         adsorbate,
-        relax_job if custom_relax_job is None else custom_relax_job,
-        static_job=(static_job if custom_static_job is None else custom_static_job)
-        if run_static
-        else None,
+        slab_relax_job,
+        static_job=slab_static_job if run_static else None,
         make_ads_kwargs=make_ads_kwargs,
     )


Test patch:
diff --git a/tests/core/calculators/espresso/test_espresso.py b/tests/core/calculators/espresso/test_espresso.py
index 73c4cde5..e44b874b 100644
--- a/tests/core/calculators/espresso/test_espresso.py
+++ b/tests/core/calculators/espresso/test_espresso.py
@@ -1,6 +1,11 @@
+from __future__ import annotations
+
+from pathlib import Path
+
+import pytest
 from ase import Atoms
 
-from quacc.calculators.espresso.espresso import Espresso
+from quacc.calculators.espresso.espresso import Espresso, EspressoTemplate
 
 
 def test_espresso_kwargs_handler():
@@ -42,3 +47,63 @@ def test_espresso_kwargs_handler():
 
     assert calc.template.binary == "pw"
     assert calc.parameters == expected_parameters
+
+
+def test_outdir_handler(tmp_path, monkeypatch):
+    monkeypatch.chdir(tmp_path)
+
+    parameters = {
+        "input_data": {
+            "control": {},
+            "system": {"outdir": "../test3/test2/test1"},
+            "electrons": {},
+            "ions": {},
+            "cell": {},
+            "rism": {},
+        }
+    }
+
+    test_path = "../test3/test2/test1"
+    fake_template = EspressoTemplate()
+    new_parameters = fake_template._outdir_handler(parameters, Path())
+    assert len(fake_template.outdirs) == 1
+    assert new_parameters["input_data"]["system"]["outdir"] == test_path
+    assert not Path(test_path).exists()
+
+    test_path = "/test3/test2/test1"
+    parameters["input_data"]["system"]["outdir"] = test_path
+    fake_template = EspressoTemplate()
+    new_parameters = fake_template._outdir_handler(parameters, Path())
+    assert len(fake_template.outdirs) == 1
+    assert new_parameters["input_data"]["system"]["outdir"] == test_path
+    assert not Path(test_path).exists()
+
+    test_path = Path("test3/test2/test1")
+    parameters["input_data"]["system"]["outdir"] = test_path
+    fake_template = EspressoTemplate()
+    new_parameters = fake_template._outdir_handler(parameters, Path())
+    assert len(fake_template.outdirs) == 0
+    assert (
+        new_parameters["input_data"]["system"]["outdir"]
+        == test_path.expanduser().resolve()
+    )
+    assert Path(tmp_path, "test3/test2/test1").exists()
+
+
+def test_bad_calculator_params():
+    calc_defaults = {
+        "input_data": {"system": {"ecutwfc": 20, "ecutrho": 80, "conv_thr": 1e-8}}
+    }
+
+    input_data = {"system": {"ecutwfc": 30}}
+
+    atoms = Atoms(symbols="LiLaOZr")
+
+    with pytest.raises(ValueError):
+        Espresso(
+            input_atoms=atoms,
+            calc_defaults=calc_defaults,
+            input_data=input_data,
+            kpts=(1, 1, 1),
+            directory="bad",
+        )
diff --git a/tests/core/recipes/espresso_recipes/test_espresso.py b/tests/core/recipes/espresso_recipes/test_espresso.py
index 77ddf987..86300b16 100644
--- a/tests/core/recipes/espresso_recipes/test_espresso.py
+++ b/tests/core/recipes/espresso_recipes/test_espresso.py
@@ -55,6 +55,120 @@ def test_static_job(tmp_path, monkeypatch):
     assert new_input_data["control"]["calculation"] == "scf"
 
 
+def test_static_job_outdir(tmp_path, monkeypatch):
+    monkeypatch.chdir(tmp_path)
+
+    pp_dir = Path(__file__).parent
+
+    copy_decompress_files([pp_dir / "Si.upf"], tmp_path)
+
+    atoms = bulk("Si")
+
+    input_data = {
+        "occupations": "smearing",
+        "smearing": "gaussian",
+        "degauss": 0.005,
+        "mixing_mode": "plain",
+        "mixing_beta": 0.6,
+        "pseudo_dir": tmp_path,
+        "conv_thr": 1.0e-6,
+        "outdir": "test2/test3/test4",
+    }
+
+    pseudopotentials = {"Si": "Si.upf"}
+
+    results = static_job(
+        atoms, input_data=input_data, pseudopotentials=pseudopotentials, kspacing=0.5
+    )
+
+    input_data = dict(construct_namelist(input_data))
+
+    assert np.allclose(results["atoms"].positions, atoms.positions, atol=1.0e-4)
+
+    assert np.allclose(results["atoms"].cell, atoms.cell, atol=1.0e-3)
+    assert (results["atoms"].symbols == atoms.symbols).all()
+
+    new_input_data = results["parameters"]["input_data"]
+
+    assert new_input_data["system"]["degauss"] == 0.005
+    assert new_input_data["system"]["occupations"] == "smearing"
+    assert new_input_data["electrons"]["conv_thr"] == 1.0e-6
+    assert new_input_data["control"]["calculation"] == "scf"
+
+
+def test_static_job_outdir_abs(tmp_path, monkeypatch):
+    monkeypatch.chdir(tmp_path)
+
+    pp_dir = Path(__file__).parent
+
+    copy_decompress_files([pp_dir / "Si.upf"], tmp_path)
+
+    atoms = bulk("Si")
+
+    input_data = {
+        "occupations": "smearing",
+        "smearing": "gaussian",
+        "degauss": 0.005,
+        "mixing_mode": "plain",
+        "mixing_beta": 0.6,
+        "pseudo_dir": tmp_path,
+        "conv_thr": 1.0e-6,
+        "outdir": Path(tmp_path, "test2").resolve(),
+    }
+
+    pseudopotentials = {"Si": "Si.upf"}
+
+    results = static_job(
+        atoms, input_data=input_data, pseudopotentials=pseudopotentials, kspacing=0.5
+    )
+
+    input_data = dict(construct_namelist(input_data))
+
+    assert np.allclose(results["atoms"].positions, atoms.positions, atol=1.0e-4)
+
+    assert np.allclose(results["atoms"].cell, atoms.cell, atol=1.0e-3)
+    assert (results["atoms"].symbols == atoms.symbols).all()
+
+    new_input_data = results["parameters"]["input_data"]
+
+    assert new_input_data["system"]["degauss"] == 0.005
+    assert new_input_data["system"]["occupations"] == "smearing"
+    assert new_input_data["electrons"]["conv_thr"] == 1.0e-6
+    assert new_input_data["control"]["calculation"] == "scf"
+
+
+def test_static_job_dir_fail(tmp_path, monkeypatch):
+    monkeypatch.chdir(tmp_path)
+
+    pp_dir = Path(__file__).parent
+
+    copy_decompress_files([pp_dir / "Si.upf"], tmp_path)
+
+    atoms = bulk("Si")
+
+    input_data = {
+        "occupations": "smearing",
+        "smearing": "gaussian",
+        "degauss": 0.005,
+        "mixing_mode": "plain",
+        "mixing_beta": 0.6,
+        "pseudo_dir": tmp_path,
+        "conv_thr": 1.0e-6,
+        "outdir": Path(tmp_path, "test2").resolve(),
+    }
+
+    pseudopotentials = {"Si": "Si.upf"}
+
+    with pytest.raises(ValueError):
+        results = static_job(
+            atoms,
+            input_data=input_data,
+            pseudopotentials=pseudopotentials,
+            kspacing=0.5,
+            directory=Path(tmp_path, "test2").resolve(),
+        )
+
+
 def test_phonon_job(tmp_path, monkeypatch):
     monkeypatch.chdir(tmp_path)