Repository: Materials-Consortia/optimade-python-tools

Issue:
Hill notation wrong (still)
Actual definition:
- If carbon is present, place it first, then hydrogen if present.
- Following hydrogen, sort elements alphabetically.
- If carbon is _not_ present, sort elements alphabetically, also if hydrogen is present.

Example of valid Hill notation formulas:
- C H<sub>4</sub> Cl O
- Cl H<sub>4</sub> O
- C Cl O

Incorrect Hill notation:
- H<sub>4</sub> Cl O

(See, e.g., the [wikipedia entry](https://en.wikipedia.org/wiki/Chemical_formula#Hill_system) for Hill notation.)

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

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

```diff
@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
- Coverage   91.77%   91.77%   -0.01%     
==========================================
  Files          62       62              
  Lines        3222     3220       -2     
==========================================
- Hits         2957     2955       -2     
  Misses        265      265              
```

| Flag | Coverage Δ | |
|---|---|---|
| project | `91.77% <100.00%> (-0.01%)` | :arrow_down: |
| validator | `64.68% <0.00%> (+0.07%)` | :arrow_up: |

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/586?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [optimade/models/structures.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/586/diff?src=pr&el=tree#diff-b3B0aW1hZGUvbW9kZWxzL3N0cnVjdHVyZXMucHk=) | `95.75% <100.00%> (-0.04%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/586?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/586?src=pr&el=footer). Last update [d48b7a8...c596ddc](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/586?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ml-evs: Actually, why don't the tests I just added fail, e.g.

https://github.com/Materials-Consortia/optimade-python-tools/blob/d48b7a80e24aad591857c386498774037bbec89c/tests/models/test_structures.py#L83-L85
ml-evs: > Actually, why don't the tests I just added fail, e.g.
> 
> https://github.com/Materials-Consortia/optimade-python-tools/blob/d48b7a80e24aad591857c386498774037bbec89c/tests/models/test_structures.py#L83-L85

Okay mb, I see that test was updated (it just looked different to my local version)
CasperWA: > Actually, why don't the tests I just added fail, e.g.
> 
> https://github.com/Materials-Consortia/optimade-python-tools/blob/d48b7a80e24aad591857c386498774037bbec89c/tests/models/test_structures.py#L83-L85

Because I updated this exact test (as mentioned in the OP and found in the `git diff`).

Patch:
diff --git a/optimade/models/structures.py b/optimade/models/structures.py
index e1aebb19..82ad7f1c 100644
--- a/optimade/models/structures.py
+++ b/optimade/models/structures.py
@@ -774,11 +774,12 @@ The properties of the species are found in the property `species`.
         expected_elements = sorted(elements)
 
         if field.name == "chemical_formula_hill":
-            # 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)
+            # Make sure C is first (and H is second, if present along with C).
+            if "C" in expected_elements:
+                expected_elements = sorted(
+                    expected_elements,
+                    key=lambda elem: {"C": "0", "H": "1"}.get(elem, elem),
+                )
 
         if any(elem not in CHEMICAL_SYMBOLS for elem in elements):
             raise ValueError(


Test patch:
diff --git a/tests/models/test_structures.py b/tests/models/test_structures.py
index 1a6b8b56..2828c608 100644
--- a/tests/models/test_structures.py
+++ b/tests/models/test_structures.py
@@ -80,8 +80,8 @@ deformities = (
         "Elements in 'chemical_formula_hill' must appear in Hill order: ['Ge', 'Si'] not ['Si', 'Ge']",
     ),
     (
-        {"chemical_formula_hill": "GeHSi"},
-        "Elements in 'chemical_formula_hill' must appear in Hill order: ['H', 'Ge', 'Si'] not ['Ge', 'H', 'Si']",
+        {"chemical_formula_hill": "HGeSi"},
+        "Elements in 'chemical_formula_hill' must appear in Hill order: ['Ge', 'H', 'Si'] not ['H', 'Ge', 'Si']",
     ),
     (
         {"chemical_formula_hill": "CGeHSi"},