Repository: Materials-Consortia/optimade-python-tools (python)

Issue:
Hill notation validation turning around C and H
In Hill notation the first element should be carbon, then hydrogen and the every other element in alphabetical order.

Currently, we are validating formulas wrongly, by expecting first hydrogen, then carbon, and finally (as it should be) every other element in alphabetical order.

Pull request description:
Fix Hill ordering validation
Fixes #581.

Add test structures to test JSON data. They are similar, with the exception that the bad structure has flipped C and H in the beginning of `chemical_formula_hill`, i.e., `HC ...` instead of the correct `CH ...`.

This also adds some `f`'s to f-strings that were missing it in the validation functions.

Patch:
diff --git a/optimade/models/structures.py b/optimade/models/structures.py
index 4203824a..e1aebb19 100644
--- a/optimade/models/structures.py
+++ b/optimade/models/structures.py
@@ -774,7 +774,8 @@ The properties of the species are found in the property `species`.
         expected_elements = sorted(elements)
 
         if field.name == "chemical_formula_hill":
-            for elem in ("C", "H"):
+            # Make sure C is first and H is second.
+            for elem in ("H", "C"):
                 if elem in expected_elements:
                     expected_elements.pop(expected_elements.index(elem))
                     expected_elements.insert(0, elem)
@@ -786,10 +787,10 @@ The properties of the species are found in the property `species`.
 
         if "1" in numbers:
             raise ValueError(
-                "Must omit proportion '1' from formula {v} in {field.name!r}"
+                f"Must omit proportion '1' from formula {v} in {field.name!r}"
             )
         if expected_elements != elements:
-            order = "Hill" if field == "chemical_formula_hill" else "alphabetical"
+            order = "Hill" if field.name == "chemical_formula_hill" else "alphabetical"
             raise ValueError(
                 f"Elements in {field.name!r} must appear in {order} order: {expected_elements} not {elements}."
             )
@@ -803,7 +804,7 @@ The properties of the species are found in the property `species`.
 
         if any(n == 1 for n in numbers):
             raise ValueError(
-                "'chemical_formula_anonymous' {v} must omit proportion '1'"
+                f"'chemical_formula_anonymous' {v} must omit proportion '1'"
             )
 
         expected_labels = ANONYMOUS_ELEMENTS[: len(elements)]


Test patch:
diff --git a/tests/models/test_data/test_bad_structures.json b/tests/models/test_data/test_bad_structures.json
index 100feb90..1204198b 100644
--- a/tests/models/test_data/test_bad_structures.json
+++ b/tests/models/test_data/test_bad_structures.json
@@ -2403,5 +2403,37 @@
       }
     ],
     "structure_features": ["assemblies"]
+  },
+  {
+    "task_id": "db/1234567",
+    "type": "structure",
+    "last_modified": {
+      "$date": "2019-06-08T05:13:37.331Z"
+    },
+    "band_gap": 1.23456,
+    "chemsys": "C-H-Cl-N-Na-O-Os-P",
+    "elements": ["C", "Cl", "H", "N", "Na", "O", "Os", "P"],
+    "nelements": 8,
+    "nsites": 7,
+    "elements_ratios": [0.09090909091, 0.36363636363, 0.09090909091, 0.09090909091, 0.09090909091, 0.09090909091, 0.09090909091, 0.09090909091],
+    "chemical_formula_reduced": "CClH4NNaOOsP",
+    "chemical_formula_hill": "H4CClNNaOOsP",
+    "chemical_formula_descriptive": "Methyl-ClNNaOOsP",
+    "formula_anonymous": "A4BCDEFGH",
+    "dimension_types": [1, 1, 1],
+    "nperiodic_dimensions": 3,
+    "lattice_vectors": [[4.0,0.0,0.0],[0.0, 4.0, 0.0],[0.0,1.0,4.0]],
+    "cartesian_site_positions": [ [0,0,0], [0,0,0], [0,0,0], [0,0,0], [0,0,0], [0,0,0], [0,0,0] ],
+    "species_at_sites": ["Cl", "O", "N", "met", "Os", "Na", "P"],
+    "species": [
+      {"name": "Cl", "chemical_symbols": ["Cl"], "concentration": [1.0] },
+      {"name": "O", "chemical_symbols": ["O"], "concentration": [1.0] },
+      {"name": "N", "chemical_symbols": ["N"], "concentration": [1.0] },
+      {"name": "met", "chemical_symbols": ["C"], "concentration": [1.0], "attached": ["H"], "nattached": [4] },
+      {"name": "Os", "chemical_symbols": ["Os"], "concentration": [1.0] },
+      {"name": "Na", "chemical_symbols": ["Na"], "concentration": [1.0] },
+      {"name": "P", "chemical_symbols": ["P"], "concentration": [1.0] }
+    ],
+    "structure_features": ["site_attachments"]
   }
 ]
diff --git a/tests/models/test_data/test_good_structures.json b/tests/models/test_data/test_good_structures.json
index a44bb018..09478ba6 100644
--- a/tests/models/test_data/test_good_structures.json
+++ b/tests/models/test_data/test_good_structures.json
@@ -158,5 +158,37 @@
       }
     ],
     "structure_features": ["assemblies", "site_attachments"]
+  },
+  {
+    "task_id": "db/1234567",
+    "type": "structure",
+    "last_modified": {
+      "$date": "2019-06-08T05:13:37.331Z"
+    },
+    "band_gap": 1.23456,
+    "chemsys": "C-H-Cl-N-Na-O-Os-P",
+    "elements": ["C", "Cl", "H", "N", "Na", "O", "Os", "P"],
+    "nelements": 8,
+    "nsites": 7,
+    "elements_ratios": [0.09090909091, 0.36363636363, 0.09090909091, 0.09090909091, 0.09090909091, 0.09090909091, 0.09090909091, 0.09090909091],
+    "chemical_formula_reduced": "CClH4NNaOOsP",
+    "chemical_formula_hill": "CH4ClNNaOOsP",
+    "chemical_formula_descriptive": "Methyl-ClNNaOOsP",
+    "formula_anonymous": "A4BCDEFGH",
+    "dimension_types": [1, 1, 1],
+    "nperiodic_dimensions": 3,
+    "lattice_vectors": [[4.0,0.0,0.0],[0.0, 4.0, 0.0],[0.0,1.0,4.0]],
+    "cartesian_site_positions": [ [0,0,0], [0,0,0], [0,0,0], [0,0,0], [0,0,0], [0,0,0], [0,0,0] ],
+    "species_at_sites": ["Cl", "O", "N", "met", "Os", "Na", "P"],
+    "species": [
+      {"name": "Cl", "chemical_symbols": ["Cl"], "concentration": [1.0] },
+      {"name": "O", "chemical_symbols": ["O"], "concentration": [1.0] },
+      {"name": "N", "chemical_symbols": ["N"], "concentration": [1.0] },
+      {"name": "met", "chemical_symbols": ["C"], "concentration": [1.0], "attached": ["H"], "nattached": [4] },
+      {"name": "Os", "chemical_symbols": ["Os"], "concentration": [1.0] },
+      {"name": "Na", "chemical_symbols": ["Na"], "concentration": [1.0] },
+      {"name": "P", "chemical_symbols": ["P"], "concentration": [1.0] }
+    ],
+    "structure_features": ["site_attachments"]
   }
 ]
diff --git a/tests/models/test_structures.py b/tests/models/test_structures.py
index 24d63877..ca72018d 100644
--- a/tests/models/test_structures.py
+++ b/tests/models/test_structures.py
@@ -54,6 +54,7 @@ deformities = (
     {"chemical_formula_hill": "SiGe"},
     {"chemical_formula_hill": "GeHSi"},
     {"chemical_formula_hill": "CGeHSi"},
+    {"chemical_formula_hill": "HCGeSi"},
     {"chemical_formula_reduced": "Ge1.0Si1.0"},
     {"chemical_formula_reduced": "GeSi2.0"},
     {"chemical_formula_reduced": "GeSi.2"},