Increasing test coverage (#162)

Adding more tests to increase test coverage. Changes include...

- monkeypatching `CalculatorExt._calculate()` rather than `calculate()` in order for the contents of `calculate()` to be tested
- a new "spin-polarized" test (actually just silicon but not assuming spin up = spin down)
- various tests for reading and writing QE files (which does not happen for during the mock workflows)

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "ase"]
 	path = ase
-	url = [email protected]:elinscott/ase_koopmans
+	url = ../../elinscott/ase_koopmans
 [submodule "quantum_espresso/q-e"]
 	path = quantum_espresso/q-e
 	url = [email protected]:QEF/q-e.git

.readthedocs.yml

@@ -22,3 +22,8 @@ python:
   version: 3.7
   install:
     - requirements: docs/requirements.txt
+
+# Make sure to checkout the ase submodule (this is a dependency required for the autodocs)
+submodules:
+  include:
+    - ase

README.rst

@@ -2,7 +2,7 @@
 koopmans
 ========
 
-| |GH Actions| |Coverage Status| |MIT License| |Documentation Status|
+| |GH Actions| |Coverage Status| |GPL License| |Documentation Status|
 
 For performing Koopmans spectral functional calculations with ``Quantum ESPRESSO``
 
@@ -124,7 +124,7 @@ For help and feedback email [email protected]
    :target: https://github.com/epfl-theos/koopmans/actions?query=branch%3Amaster
 .. |Coverage Status| image:: https://img.shields.io/codecov/c/gh/epfl-theos/koopmans/master?logo=codecov
    :target: https://codecov.io/gh/epfl-theos/koopmans
-.. |MIT License| image:: https://img.shields.io/badge/license-MIT-blue.svg
+.. |GPL License| image:: https://img.shields.io/badge/license-GPL-blue
    :target: https://github.com/epfl-theos/koopmans/blob/master/LICENSE
 .. |Documentation Status| image:: https://readthedocs.org/projects/koopmans/badge/?version=latest
    :target: https://koopmans-functionals.org/en/latest/?badge=latest

codecov.yml

@@ -5,7 +5,7 @@ codecov:
 coverage:
   precision: 2
   round: down
-  range: "50...80"
+  range: "50..90"
 
   status:
     project: off

koopmans/calculators/_koopmans_cp.py

@@ -353,6 +353,7 @@ def read_alphas(self) -> List[List[float]]:
 
         flat_alphas = utils.read_alpha_file(self.directory)
 
+        assert isinstance(self.parameters, settings.KoopmansCPSettingsDict)
         return convert_flat_alphas_for_kcp(flat_alphas, self.parameters)
 
     # The following functions enable DOS generation via ase.dft.dos.DOS(<KoopmansCPCalculator object>)
@@ -511,9 +512,10 @@ def convert_flat_alphas_for_kcp(flat_alphas: List[float],
     # Here we reorder this into a nested list indexed by [i_spin][i_orbital]
     if parameters.nspin == 2:
         nbnd = len(flat_alphas) // 2
+        nelec = parameters.nelec if parameters.nelec else parameters.nelup + parameters.neldw
         alphas = [flat_alphas[:parameters.nelup]
-                  + flat_alphas[parameters.nelec:(nbnd + parameters.neldw)],
-                  flat_alphas[parameters.nelup:parameters.nelec]
+                  + flat_alphas[nelec:(nbnd + parameters.neldw)],
+                  flat_alphas[parameters.nelup:nelec]
                   + flat_alphas[(nbnd + parameters.neldw):]]
     else:
         alphas = [flat_alphas]

koopmans/calculators/_pw.py

@@ -36,12 +36,12 @@ def __init__(self, atoms: Atoms, *args, **kwargs):
             self.command = ParallelCommandWithPostfix(os.environ.get(
                 'ASE_ESPRESSO_COMMAND', str(bin_directory) + os.path.sep + self.command))
 
-    def calculate(self):
+    def _calculate(self):
         if self.parameters.calculation == 'bands':
             if not isinstance(self.parameters.kpts, BandPath):
                 raise KeyError('You are running a calculation that requires a kpoint path; please provide a BandPath '
                                'as the kpts parameter')
-        super().calculate()
+        super()._calculate()
 
         if isinstance(self.parameters.kpts, BandPath):
             # Add the bandstructure to the results. This is very un-ASE-y and might eventually be replaced

koopmans/calculators/_utils.py

@@ -76,6 +76,9 @@ class CalculatorExt():
     ext_out: str = ''
 
     def __init__(self, skip_qc: bool = False, **kwargs: Any):
+        # Remove arguments that should not be treated as QE keywords
+        kwargs.pop('directory', None)
+
         # Handle any recognized QE keywords passed as arguments
         self.parameters.update(**kwargs)
 
@@ -84,7 +87,7 @@ def __init__(self, skip_qc: bool = False, **kwargs: Any):
         self.skip_qc = skip_qc
 
     @property
-    def parameters(self):
+    def parameters(self) -> settings.SettingsDict:
         if not hasattr(self, '_parameters'):
             raise ValueError(f'{self}.parameters has not yet been set')
         return self._parameters
@@ -164,13 +167,14 @@ def read_input(self, input_file: Optional[Path] = None):
 
     def check_code_is_installed(self):
         # Checks the corresponding code is installed
-        if self.command.path == '':
+        if self.command.path == Path():
             executable_with_path = utils.find_executable(self.command.executable)
             if executable_with_path is None:
                 raise OSError(f'{self.command.executable} is not installed')
             self.command.path = executable_with_path.rsplit('/', 1)[0] + '/'
         else:
-            assert (self.command.path / self.command.executable).is_file
+            if not (self.command.path / self.command.executable).is_file():
+                raise OSError(f'{self.command.executable} is not installed')
         return
 
     def write_alphas(self):

koopmans/cli/main.py

@@ -13,7 +13,7 @@
 
 
 def main():
-    # Automatically constructing a list of workflow keywords for 'run_koopmans.py --help'
+    # Automatically constructing a list of workflow keywords for 'koopmans --help'
     # from valid_settings
     epilog = ''
     wf_settings = WorkflowSettingsDict()

koopmans/commands.py

@@ -6,6 +6,7 @@
 
 import os
 from pathlib import Path
+from typing import Union
 
 
 class Command(object):
@@ -19,7 +20,7 @@ class Command(object):
     """
 
     def __init__(self, value=None, **kwargs):
-        self.path: Path = Path()
+        self._path = Path()
         self.executable: str = ''
         self._flags: str = ''
         self.suffix: str = ''
@@ -55,6 +56,14 @@ def __set__(self, value):
     def __repr__(self):
         return ' '.join([str(self.path / self.executable), self.flags, self.suffix]).replace('  ', ' ')
 
+    @property
+    def path(self) -> Path:
+        return self._path
+
+    @path.setter
+    def path(self, value: Union[Path, str]):
+        self._path = Path(value)
+
     @property
     def flags(self) -> str:
         return self._flags

koopmans/testing/_generate_benchmarks.py

@@ -21,7 +21,36 @@
 
 
 class BenchmarkGenCalc():
+
+    def _calculate(self):
+        '''
+        After running, gemerate a benchmark json file
+        '''
+
+        # Run the calculation
+        super()._calculate()
+
+        # Write out the calculator itself to file
+        # Make sure we store all paths as relative paths
+        tmp, self.parameters.use_relative_paths = self.parameters.use_relative_paths, True
+
+        # Don't store the walltime
+        self.results.pop('walltime', None)
+
+        # Save the calculator as an encoded json in the benchmarks directory
+        with open(benchmark_filename(self), 'w') as fd:
+            write_encoded_json(self, fd)
+
+        # Restore the behaviour of use_relative_paths
+        self.parameters.use_relative_paths = tmp
+
     def calculate(self):
+        '''
+        Before running super().calculate(), construct a list of files generated by a calculation. Note that we do this
+        as an extension of calculate() and not _calculate() because in some instances (e.g. the KoopmansCPCalculator)
+        calculate() generates additional files
+        '''
+
         # Before running the calculation, make a list of the files that exist
         files_before = find_subfiles_of_calc(self)
 
@@ -41,20 +70,6 @@ def calculate(self):
             wfc_prefix = os.path.relpath(self.parameters.outdir, self.directory) + f'/{self.parameters.prefix}.wfc'
             modified_files = [f for f in modified_files if not f.startswith(wfc_prefix)]
 
-        # Write out the calculator itself to file
-        # Make sure we store all paths as relative paths
-        tmp, self.parameters.use_relative_paths = self.parameters.use_relative_paths, True
-
-        # Don't store the walltime
-        self.results.pop('walltime', None)
-
-        # Save the calculator as an encoded json in the benchmarks directory
-        with open(benchmark_filename(self), 'w') as fd:
-            write_encoded_json(self, fd)
-
-        # Restore the behaviour of use_relative_paths
-        self.parameters.use_relative_paths = tmp
-
         # Write out the modified files to the "_metadata.json" file
         fname = metadata_filename(self)
         with open(fname, 'w') as fd:

koopmans/testing/_mock.py

@@ -2,10 +2,11 @@
 import json
 import os
 from pathlib import Path
-from typing import List, Optional, Union
+from typing import List, Union
 
 import numpy as np
 
+from ase.atoms import Atoms
 from koopmans import projections, utils
 from koopmans.calculators import (
     Calc,
@@ -36,8 +37,13 @@ def write_mock_file(filename: Union[Path, str], written_by: str):
                        'written_by': written_by}, fd)
 
 
-class MockCalc(ABC):
-    def calculate(self):
+class MockCalc:
+    atoms: Atoms
+    prefix: str
+    ext_in: str
+    ext_out: str
+
+    def _calculate(self):
         # Write the input file
         self.write_input(self.atoms)
 
@@ -101,11 +107,11 @@ def is_complete(self):
         return (self.directory / f'{self.prefix}{self.ext_out}').is_file()
 
     def check_code_is_installed(self):
-        # Don't check if the code is installed
-        return
-
-    def check_convergence(self):
-        # Monkeypatched version of check_convergence to avoid any convergence check
+        try:
+            super().check_code_is_installed()
+        except OSError as e:
+            # The only valid error to encounter is that the code in question is not installed
+            assert str(e) == f'{self.command.executable} is not installed'
         return
 
     def read_results(self) -> None:
@@ -144,7 +150,7 @@ class MockWann2KCPCalculator(MockCalc, Wann2KCPCalculator):
 
 
 class MockUnfoldAndInterpolateCalculator(MockCalc, UnfoldAndInterpolateCalculator):
-    def write_results(self):
+    def write_results(self, *args, **kwargs):
         # Do nothing when it goes to write out the results (because this relies on self.bg, among others)
         return
 

koopmans/utils/_io.py

@@ -301,7 +301,7 @@ def read_wannier_centers_file(fname: Path):
         else:
             symbols.append(line.split()[0])
             positions.append([float(x) for x in line.split()[1:]])
-    return centers, Atoms(symbols=symbols, positions=positions)
+    return centers, Atoms(symbols=symbols, positions=positions, pbc=True)
 
 
 def write_wannier_centers_file(fname: Path, centers: List[List[float]], atoms: Atoms):

koopmans/utils/_os.py

@@ -106,7 +106,10 @@ def set_env(**environ):
         os.environ.update(old_environ)
 
 
-def find_executable(program: Path):
+def find_executable(program: Union[Path, str]):
+    if isinstance(program, str):
+        program = Path(program)
+
     # Equivalent to the unix command "which"
     def is_exe(fpath: Path):
         return fpath.is_file() and os.access(fpath, os.X_OK)

setup.py

@@ -8,7 +8,8 @@
 from setuptools import find_packages, setup
 
 with open('koopmans/__init__.py', 'r') as f:
-    version = f.readlines()[1].split('=')[-1].strip()
+    [versionline] = [line for line in f.readlines() if 'version' in line]
+    version = versionline.split('=')[-1].strip().strip("'")
 
 with open('requirements/requirements.txt', 'r') as f:
     requirements = [line.strip() for line in f.readlines()]

tests/benchmarks/test_singlepoint_si_ki_dscf0-calc_alpha-ki.json → tests/benchmarks/test_singlepoint_si_ki_dscf_Fa0-calc_alpha-ki.json

@@ -5,7 +5,6 @@
   },
   "conv_thr": 6.4e-08,
   "esic_conv_thr": 6.4e-08,
-  "ecutwfc": 40.0,
   "nelec": 64,
   "tot_charge": 0,
   "tot_magnetization": 0,
@@ -14,6 +13,7 @@
   "pseudo_dir": {
    "__path__": "../../../../pseudos/sg15_v1.2/pbe"
   },
+  "ecutwfc": 40.0,
   "nbnd": 64,
   "pseudopotentials": {
    "Si": "Si_ONCV_PBE-1.2.upf"
@@ -34184,7 +34184,7 @@
   ]
  },
  "_directory": {
-  "__path__": "/home/elinscott/code/koopmans/tests/tmp/test_singlepoint_si_ki_dscf0/calc_alpha"
+  "__path__": "/home/elinscott/code/koopmans/tests/tmp/test_singlepoint_si_ki_dscf_Fa0/calc_alpha"
  },
  "prefix": "ki",
  "name": "benchgenkoopmanscpcalculator",

tests/benchmarks/test_singlepoint_si_ki_dscf0-calc_alpha-orbital_32-dft_n-1.json → tests/benchmarks/test_singlepoint_si_ki_dscf_Fa0-calc_alpha-orbital_32-dft_n-1.json

@@ -5,7 +5,6 @@
   },
   "conv_thr": 6.4000000000000006e-06,
   "esic_conv_thr": 6.4000000000000006e-06,
-  "ecutwfc": 40.0,
   "nelec": 64,
   "tot_charge": 0,
   "tot_magnetization": 0,
@@ -14,6 +13,7 @@
   "pseudo_dir": {
    "__path__": "../../../../../pseudos/sg15_v1.2/pbe"
   },
+  "ecutwfc": 40.0,
   "pseudopotentials": {
    "Si": "Si_ONCV_PBE-1.2.upf"
   },
@@ -5068,7 +5068,7 @@
   ]
  },
  "_directory": {
-  "__path__": "/home/elinscott/code/koopmans/tests/tmp/test_singlepoint_si_ki_dscf0/calc_alpha/orbital_32"
+  "__path__": "/home/elinscott/code/koopmans/tests/tmp/test_singlepoint_si_ki_dscf_Fa0/calc_alpha/orbital_32"
  },
  "prefix": "dft_n-1",
  "name": "benchgenkoopmanscpcalculator",

tests/benchmarks/test_singlepoint_si_ki_dscf0-calc_alpha-orbital_33-dft_n+1.json → tests/benchmarks/test_singlepoint_si_ki_dscf_Fa0-calc_alpha-orbital_33-dft_n+1.json

@@ -5,7 +5,6 @@
   },
   "conv_thr": 6.4000000000000006e-06,
   "esic_conv_thr": 6.4000000000000006e-06,
-  "ecutwfc": 40.0,
   "nelec": 65,
   "tot_charge": 0,
   "tot_magnetization": 0,
@@ -14,6 +13,7 @@
   "pseudo_dir": {
    "__path__": "../../../../../pseudos/sg15_v1.2/pbe"
   },
+  "ecutwfc": 40.0,
   "pseudopotentials": {
    "Si": "Si_ONCV_PBE-1.2.upf"
   },
@@ -5233,7 +5233,7 @@
   ]
  },
  "_directory": {
-  "__path__": "/home/elinscott/code/koopmans/tests/tmp/test_singlepoint_si_ki_dscf0/calc_alpha/orbital_33"
+  "__path__": "/home/elinscott/code/koopmans/tests/tmp/test_singlepoint_si_ki_dscf_Fa0/calc_alpha/orbital_33"
  },
  "prefix": "dft_n+1",
  "name": "benchgenkoopmanscpcalculator",