Repository: Materials-Consortia/optimade-python-tools

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.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582?src=pr&el=h1) Report
> Merging [#582](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582?src=pr&el=desc) into [master](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/commit/a6001924840ced0efe84198c1d582b9fc49c2eb8?el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582/graphs/tree.svg?width=650&height=150&src=pr&token=UJAtmqkZZO)](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #582   +/-   ##
=======================================
  Coverage   91.77%   91.77%           
=======================================
  Files          62       62           
  Lines        3221     3221           
=======================================
  Hits         2956     2956           
  Misses        265      265           
```

| Flag | Coverage Δ | |
|---|---|---|
| project | `91.77% <100.00%> (ø)` | |
| validator | `64.63% <0.00%> (-0.04%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [optimade/models/structures.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582/diff?src=pr&el=tree#diff-b3B0aW1hZGUvbW9kZWxzL3N0cnVjdHVyZXMucHk=) | `95.79% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582?src=pr&el=footer). Last update [a600192...1275fe2](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/582?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ml-evs: > Annoyingly if I hadn't made the extra `field` vs `field.name` mistake, then our tests would have caught this.

One extra comment here, we could make the deformity tests more robust by adding the expected error message for all of them too, this would have caught it as the validation of Hill formula cases was failing because of being out of alphabetical order rather than Hill. This will be a bit laborious but I can raise an issue and try to find the time.
CasperWA: > > Annoyingly if I hadn't made the extra `field` vs `field.name` mistake, then our tests would have caught this.
> 
> One extra comment here, we could make the deformity tests more robust by adding the expected error message for all of them too, this would have caught it as the validation of Hill formula cases was failing because of being out of alphabetical order rather than Hill. This will be a bit laborious but I can raise an issue and try to find the time.

If it doesn't take too much time it's fine, otherwise it's also fine :) There are more important things and I think we caught most of it now.
CasperWA: > This is my bad, thanks for the fix @CasperWA. Annoyingly if I hadn't made the extra `field` vs `field.name` mistake, then our tests would have caught this.

No worries, no harm done.

> Don't suppose you know any linting tools that detect missing `f`'s in what it thinks are f-strings?

No, unfortunately only the opposite.

> I've just committed one tiny extra test case (which I thought I had originally included). Please take a look at it then squash me out of this PR.

Will keep you in, depending on the test ;)

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"},