Repository: Materials-Consortia/optimade-python-tools
Pull request #297 (2020-06-10 07:11:30)

Adding jarvis-tools structures

CasperWA: > Yes, I think so too. But we do not have the vacancy in the Specie object. We mention in the 'test_special_species' function. We define a separate Vacancy object.

So the `chemical_symbols` is a list of chemical elements (case sensitive), but can also include `"X"` as an unknown element, which isn't a vacancy, and then of course also `"vacancy"`, which will represent a vacancy.
If `jarvis.core.atoms.Atoms` cannot handle these (`"X"`), you should try to handle that before returning the `Atoms` object.
For `"vacancy"` you're already doing this, since you're checking for `"disorder"` and raising if found in `structure_features`, so that's excellent! :) However, a structure may also have a single `species` with `chemical_symbols = ["X"]`, which isn't caught by this check. Would this break jarvis' Atoms?

Actually, we are not properly testing for this at the moment! So I think I'll create a separate PR adding this test.
For this PR, I consider it good-enough at this time, but the adapter may have to be updated for the separate PR.
If you want to include the "fix" already in this PR that's great! :) Otherwise I would consider it fine to add it in the separate PR, including the other fixes that would need to be done (for, e.g., ASE).
CasperWA: @knc6 I've been looking a bit into the code of jarvis, and it seems the `Specie` class you're wrapping the entries of `Atoms.elements` in takes a `str` as initializer.
I would suggest to then initialize your `Atoms` instance in the adapter, providing `elements` with a generated list of OPTIMADE species' `name` key, i.e.:

```python3
Atoms(
    elements=[specie.name for specie in attributes.species],
    # ... other instantiating attributes ...
)
```

Do you think this makes sense? Instead of your `Specie` class having to initalize with `symbol` being a Python dictionary instead of a string (which would currently be the case)?
CasperWA: @knc6 I have updated your test here to take into account structures with special non-disordered species (i.e., updating it according to the newly added tests in #305.
I hope it's all right? If not, please make the appropriate changes on top.
If you're having trouble getting a local version matching the one here on github, you can first do `git fetch --all -p` followed by (as long as you have checked out this branch locally) `git reset --hard knc6/master`. This will reset your local version of the branch to the one on GitHub disregarding any differences.
knc6: @CasperWA I changed the jarvis.py in the adapters/structures accordingly. Your changes makes sense to me. Atoms constructor should be able to take 'X' or any other character. It will only fail, if we ask for some chemical properties such as Specie('X').atomic_mass.

Patch:
diff --git a/optimade/adapters/structures/__init__.py b/optimade/adapters/structures/__init__.py
index 048800be..6922615c 100644
--- a/optimade/adapters/structures/__init__.py
+++ b/optimade/adapters/structures/__init__.py
@@ -6,6 +6,7 @@ from .ase import get_ase_atoms
 from .cif import get_cif
 from .proteindatabank import get_pdb, get_pdbx_mmcif
 from .pymatgen import get_pymatgen
+from .jarvis import get_jarvis_atoms
 
 
 __all__ = ("Structure",)
@@ -24,4 +25,5 @@ class Structure(EntryAdapter):
         "pdb": get_pdb,
         "pdbx_mmcif": get_pdbx_mmcif,
         "pymatgen": get_pymatgen,
+        "jarvis": get_jarvis_atoms,
     }
diff --git a/optimade/adapters/structures/jarvis.py b/optimade/adapters/structures/jarvis.py
new file mode 100644
index 00000000..c1520001
--- /dev/null
+++ b/optimade/adapters/structures/jarvis.py
@@ -0,0 +1,43 @@
+from warnings import warn
+from optimade.models import StructureResource as OptimadeStructure
+from optimade.adapters.exceptions import ConversionError
+from optimade.adapters.structures.utils import pad_positions
+
+try:
+    from jarvis.core.atoms import Atoms
+except (ImportError, ModuleNotFoundError):
+    Atoms = None
+    JARVIS_NOT_FOUND = "jarvis-tools package not found, cannot convert structure to a JARVIS Atoms. Visit https://github.com/usnistgov/jarvis"
+
+
+__all__ = ("get_jarvis_atoms",)
+
+
+def get_jarvis_atoms(optimade_structure: OptimadeStructure) -> Atoms:
+    """ Get jarvis Atoms from OPTIMADE structure
+
+    NOTE: Cannot handle partial occupancies
+
+    :param optimade_structure: OPTIMADE structure
+    :return: jarvis.core.Atoms
+    """
+    if globals().get("Atoms", None) is None:
+        warn(JARVIS_NOT_FOUND)
+        return None
+
+    attributes = optimade_structure.attributes
+
+    # Cannot handle partial occupancies
+    if "disorder" in attributes.structure_features:
+        raise ConversionError(
+            "jarvis-tools cannot handle structures with partial occupancies."
+        )
+
+    cartesian_site_positions, _ = pad_positions(attributes.cartesian_site_positions)
+
+    return Atoms(
+        lattice_mat=attributes.lattice_vectors,
+        elements=[specie.name for specie in attributes.species],
+        coords=cartesian_site_positions,
+        cartesian=True,
+    )
diff --git a/setup.py b/setup.py
index 5c663277..96f70e00 100644
--- a/setup.py
+++ b/setup.py
@@ -16,6 +16,7 @@ ase_deps = ["ase~=3.19"]
 cif_deps = ["numpy~=1.18"]
 pdb_deps = cif_deps
 pymatgen_deps = ["pymatgen~=2020.3"]
+jarvis_deps = ["jarvis-tools~=2020.6"]
 client_deps = cif_deps
 
 # General
@@ -27,7 +28,15 @@ testing_deps = [
     "jsondiff",
 ] + server_deps
 dev_deps = ["pylint", "black", "pre-commit", "invoke"] + testing_deps + client_deps
-all_deps = dev_deps + django_deps + elastic_deps + aiida_deps + ase_deps + pymatgen_deps
+all_deps = (
+    dev_deps
+    + django_deps
+    + elastic_deps
+    + aiida_deps
+    + ase_deps
+    + pymatgen_deps
+    + jarvis_deps
+)
 
 setup(
     name="optimade",
@@ -76,6 +85,7 @@ setup(
         "cif": cif_deps,
         "pdb": pdb_deps,
         "pymatgen": pymatgen_deps,
+        "jarvis": jarvis_deps,
     },
     entry_points={
         "console_scripts": ["optimade_validator=optimade.validator:validate"]


Test patch:
diff --git a/tests/adapters/structures/test_jarvis.py b/tests/adapters/structures/test_jarvis.py
new file mode 100644
index 00000000..929ba425
--- /dev/null
+++ b/tests/adapters/structures/test_jarvis.py
@@ -0,0 +1,53 @@
+# pylint: disable=import-error
+import pytest
+
+from .utils import get_min_ver
+
+min_ver = get_min_ver("jarvis-tools")
+jarvis = pytest.importorskip(
+    "jarvis",
+    minversion=min_ver,
+    reason=f"jarvis-tools must be installed with minimum version {min_ver} for these tests to"
+    " be able to run",
+)
+
+from jarvis.core.atoms import Atoms
+from optimade.adapters import Structure
+from optimade.adapters.exceptions import ConversionError
+from optimade.adapters.structures.jarvis import get_jarvis_atoms
+
+
+def test_successful_conversion(RAW_STRUCTURES):
+    """Make sure its possible to convert"""
+    for structure in RAW_STRUCTURES:
+        assert isinstance(get_jarvis_atoms(Structure(structure)), Atoms)
+
+
+def test_null_positions(null_position_structure):
+    """Make sure null positions are handled"""
+    assert isinstance(get_jarvis_atoms(null_position_structure), Atoms)
+
+
+def test_null_lattice_vectors(null_lattice_vector_structure):
+    """Make sure null lattice vectors are handled"""
+    assert isinstance(get_jarvis_atoms(null_lattice_vector_structure), Atoms)
+
+
+def test_special_species(SPECIAL_SPECIES_STRUCTURES):
+    """Make sure vacancies and non-chemical symbols ("X") are handled"""
+    for special_structure in SPECIAL_SPECIES_STRUCTURES:
+        structure = Structure(special_structure)
+
+        # Since all the special species structure only have a single species, this works fine.
+        if len(structure.species[0].chemical_symbols) > 1:
+            # If the structure is disordered (has partial occupancies of any kind),
+            # jarvis-tools cannot convert the structure
+            with pytest.raises(
+                ConversionError,
+                match="jarvis-tools cannot handle structures with partial occupancies",
+            ):
+                get_jarvis_atoms(structure)
+        else:
+            # No partial occupancies, just special/non-standard species.
+            # jarvis-tools should convert these structure fine enough.
+            assert isinstance(get_jarvis_atoms(structure), Atoms)
diff --git a/tests/adapters/structures/test_structures.py b/tests/adapters/structures/test_structures.py
index 3cea1eab..1e950c9f 100644
--- a/tests/adapters/structures/test_structures.py
+++ b/tests/adapters/structures/test_structures.py
@@ -8,6 +8,7 @@ try:
     import ase  # noqa: F401
     import numpy  # noqa: F401
     import pymatgen  # noqa: F401
+    import jarvis  # noqa: F401
 except ImportError:
     all_modules_found = False
 else:
@@ -107,10 +108,11 @@ class TestStructure:
             "ase": ["ase"],
             "numpy": ["cif", "pdb", "pdbx_mmcif"],
             "pymatgen": ["pymatgen"],
+            "jarvis": ["jarvis"],
         }
 
         modules_to_test = []
-        for module in ("aiida", "ase", "numpy", "pymatgen"):
+        for module in ("aiida", "ase", "numpy", "pymatgen", "jarvis"):
             try:
                 importlib.import_module(module)
             except (ImportError, ModuleNotFoundError):