Repository: OpenFreeEnergy/openfe

Issue:
atom mapping bonds seem red too often
In the mapping visualisation below, 19:14 is in the mapping, which is marked blue as a transmutation.  The bond is coloured red as not mapped, but arguably should be blue as something which is changing?

![index](https://user-images.githubusercontent.com/9249543/212974973-d1343912-35b2-4fd8-9521-c82a33f765fe.png)


Discussion:
richardjgowers: one thing that I'm still not quite happy about is that atom 20(left)/21(right) is unchanged, but this is hard to see behind the bond highlight.
richardjgowers: A more simple example, what the vis looks like with and without element changes allowed.  

![index3](https://user-images.githubusercontent.com/9249543/213472243-07e74888-70c0-4b98-9bf3-8ad2c0a54497.png)
![index4](https://user-images.githubusercontent.com/9249543/213472251-aa7b1749-6486-4f99-aae8-7a2bdcd16d46.png)

codecov[bot]: # [Codecov](https://codecov.io/gh/OpenFreeEnergy/openfe/pull/241?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy) Report
Base: **92.68**% // Head: **92.65**% // Decreases project coverage by **`-0.04%`** :warning:
> Coverage data is based on head [(`09adb78`)](https://codecov.io/gh/OpenFreeEnergy/openfe/pull/241?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy) compared to base [(`82a6a6d`)](https://codecov.io/gh/OpenFreeEnergy/openfe/commit/82a6a6db4c22cc5b30ddf26253db10130a24662f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
- Coverage   92.68%   92.65%   -0.04%     
==========================================
  Files          62       62              
  Lines        4377     4382       +5     
==========================================
+ Hits         4057     4060       +3     
- Misses        320      322       +2     
```


| [Impacted Files](https://codecov.io/gh/OpenFreeEnergy/openfe/pull/241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy) | Coverage Δ | |
|---|---|---|
| [openfe/tests/utils/test\_visualization.py](https://codecov.io/gh/OpenFreeEnergy/openfe/pull/241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy#diff-b3BlbmZlL3Rlc3RzL3V0aWxzL3Rlc3RfdmlzdWFsaXphdGlvbi5weQ==) | `100.00% <100.00%> (ø)` | |
| [openfe/utils/visualization.py](https://codecov.io/gh/OpenFreeEnergy/openfe/pull/241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy#diff-b3BlbmZlL3V0aWxzL3Zpc3VhbGl6YXRpb24ucHk=) | `100.00% <100.00%> (ø)` | |
| [openfe/utils/custom\_typing.py](https://codecov.io/gh/OpenFreeEnergy/openfe/pull/241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy#diff-b3BlbmZlL3V0aWxzL2N1c3RvbV90eXBpbmcucHk=) | `84.61% <0.00%> (-15.39%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/OpenFreeEnergy/openfe/pull/241?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy).


Patch:
diff --git a/openfe/utils/visualization.py b/openfe/utils/visualization.py
index f5b85c9f..ad3e6d6a 100644
--- a/openfe/utils/visualization.py
+++ b/openfe/utils/visualization.py
@@ -9,6 +9,12 @@ from rdkit.Chem import AllChem
 from openfe.utils.custom_typing import RDKitMol
 
 
+# highlight core element changes differently from unique atoms
+# RGBA color value needs to be between 0 and 1, so divide by 255
+RED = (220 / 255, 50 / 255, 32 / 255, 1.0)
+BLUE = (0.0, 90 / 255, 181 / 255, 1.0)
+
+
 def _match_elements(
         mol1: RDKitMol, idx1: int, mol2: RDKitMol, idx2: int
 ) -> bool:
@@ -57,14 +63,15 @@ def _get_unique_bonds_and_atoms(
     -------
     uniques : dict
         Dictionary containing; unique atoms ("atoms"), new elements
-        ("elements"), and bonds involved with either of the previous two
-        ("bonds") for molecule 1.
+        ("elements"), deleted bonds ("bond_deletions) and altered bonds
+        ("bond_changes) for molecule 1.
     """
 
     uniques: Dict[str, set] = {
         "atoms": set(),  # atoms which fully don't exist in molB
         "elements": set(),  # atoms which exist but change elements in molB
-        "bonds": set(),  # bonds involving either unique atoms or elements
+        "bond_deletions": set(),  # bonds which are removed
+        "bond_changes": set(),  # bonds which change
     }
 
     for at in mol1.GetAtoms():
@@ -79,7 +86,11 @@ def _get_unique_bonds_and_atoms(
         for at in chain(uniques["atoms"], uniques["elements"]):
             if at in bond_at_idxs:
                 bond_idx = bond.GetIdx()
-                uniques["bonds"].add(bond_idx)
+
+                if any(i in uniques["atoms"] for i in bond_at_idxs):
+                    uniques["bond_deletions"].add(bond_idx)
+                else:
+                    uniques["bond_changes"].add(bond_idx)
 
     return uniques
 
@@ -90,6 +101,7 @@ def _draw_molecules(
     atoms_list: Collection[Set[int]],
     bonds_list: Collection[Set[int]],
     atom_colors: Collection[Dict[Any, Tuple[float, float, float, float]]],
+    bond_colors: Collection[dict[int, tuple[float, float, float, float]]],
     highlight_color: Tuple[float, float, float, float],
     atom_mapping: Optional[Dict[Tuple[int, int], Dict[int, int]]] = None,
 ) -> str:
@@ -114,6 +126,8 @@ def _draw_molecules(
         dict containing a mapping of RDKit atom to color, expressed as an
         RGBA tuple, for atoms that need special coloring (e.g., element
         changes)
+    bond_colors: Collection[dict[int, tuple[float, float, float, float]]]
+        one dict for each molecule, each dict mapping
     highlight_color: Tuple[float, float, float, float]
         RGBA tuple for the default highlight color used in the mapping
         visualization
@@ -156,6 +170,7 @@ def _draw_molecules(
         highlightAtoms=atoms_list,
         highlightBonds=bonds_list,
         highlightAtomColors=atom_colors,
+        highlightBondColors=bond_colors,
     )
     d2d.FinishDrawing()
     return d2d.GetDrawingText()
@@ -197,18 +212,19 @@ def draw_mapping(
         mol1_uniques["atoms"] | mol1_uniques["elements"],
         mol2_uniques["atoms"] | mol2_uniques["elements"],
     ]
+    bonds_list = [
+        mol1_uniques["bond_deletions"] | mol1_uniques["bond_changes"],
+        mol2_uniques["bond_deletions"] | mol2_uniques["bond_changes"],
+    ]
 
-    # highlight core element changes differently from unique atoms
-    # RGBA color value needs to be between 0 and 1, so divide by 255
-    red = (220 / 255, 50 / 255, 32 / 255, 1.0)
-    blue = (0.0, 90 / 255, 181 / 255, 1.0)
-
-    at1_colors = {at: blue for at in mol1_uniques["elements"]}
-    at2_colors = {at: blue for at in mol2_uniques["elements"]}
+    at1_colors = {at: BLUE for at in mol1_uniques["elements"]}
+    at2_colors = {at: BLUE for at in mol2_uniques["elements"]}
+    bd1_colors = {bd: BLUE for bd in mol1_uniques["bond_changes"]}
+    bd2_colors = {bd: BLUE for bd in mol2_uniques["bond_changes"]}
 
     atom_colors = [at1_colors, at2_colors]
+    bond_colors = [bd1_colors, bd2_colors]
 
-    bonds_list = [mol1_uniques["bonds"], mol2_uniques["bonds"]]
 
     return _draw_molecules(
         d2d,
@@ -216,7 +232,8 @@ def draw_mapping(
         atoms_list=atoms_list,
         bonds_list=bonds_list,
         atom_colors=atom_colors,
-        highlight_color=red,
+        bond_colors=bond_colors,
+        highlight_color=RED,
         atom_mapping={(0, 1): mol1_to_mol2},
     )
 
@@ -244,18 +261,18 @@ def draw_one_molecule_mapping(mol1_to_mol2, mol1, mol2, d2d=None):
     """
     uniques = _get_unique_bonds_and_atoms(mol1_to_mol2, mol1, mol2)
     atoms_list = [uniques["atoms"] | uniques["elements"]]
-    bonds_list = [uniques["bonds"]]
-    red = (220 / 255, 50 / 255, 32 / 255, 1.0)
-    blue = (0, 90 / 255, 181 / 255, 1.0)
+    bonds_list = [uniques["bond_deletions"] | uniques["bond_changes"]]
 
-    atom_colors = [{at: blue for at in uniques["elements"]}]
+    atom_colors = [{at: BLUE for at in uniques["elements"]}]
+    bond_colors = [{bd: BLUE for bd in uniques["bond_changes"]}]
 
     return _draw_molecules(d2d,
                            [mol1],
                            atoms_list,
                            bonds_list,
                            atom_colors,
-                           red,
+                           bond_colors,
+                           RED,
                            )
 
 
@@ -281,5 +298,6 @@ def draw_unhighlighted_molecule(mol, d2d=None):
         atoms_list=[[]],
         bonds_list=[[]],
         atom_colors=[{}],
+        bond_colors=[{}],
         highlight_color=red,
     )


Test patch:
diff --git a/openfe/tests/utils/test_visualization.py b/openfe/tests/utils/test_visualization.py
index 17558ac6..c30288b0 100644
--- a/openfe/tests/utils/test_visualization.py
+++ b/openfe/tests/utils/test_visualization.py
@@ -69,11 +69,11 @@ def benzene_phenol_mapping(benzene_transforms, maps):
     return mapping, mol1, mol2
 
 
-@pytest.mark.parametrize('molname, atoms, elems, bonds', [
-    ['phenol', {10, }, {12, }, {10, 12}],
-    ['anisole', {0, 1, 3, 4}, {2, }, {0, 1, 2, 3, 13}]
+@pytest.mark.parametrize('molname, atoms, elems, bond_changes, bond_deletions', [
+    ['phenol', {10, }, {12, }, {10, }, {12, }],
+    ['anisole', {0, 1, 3, 4}, {2, }, {13, }, {0, 1, 2, 3}]
 ])
-def test_benzene_to_phenol_uniques(molname, atoms, elems, bonds,
+def test_benzene_to_phenol_uniques(molname, atoms, elems, bond_changes, bond_deletions,
                                    benzene_transforms, maps):
     mol1 = benzene_transforms['benzene']
     mol2 = benzene_transforms[molname]
@@ -89,7 +89,7 @@ def test_benzene_to_phenol_uniques(molname, atoms, elems, bonds,
     # H->O
     assert uniques['elements'] == {10, }
     # One bond involved
-    assert uniques['bonds'] == {10, }
+    assert uniques['bond_changes'] == {10, }
 
     # invert and check the molB uniques
     inv_map = {v: k for k, v in mapping.items()}
@@ -99,7 +99,8 @@ def test_benzene_to_phenol_uniques(molname, atoms, elems, bonds,
 
     assert uniques['atoms'] == atoms
     assert uniques['elements'] == elems
-    assert uniques['bonds'] == bonds
+    assert uniques['bond_changes'] == bond_changes
+    assert uniques['bond_deletions'] == bond_deletions
 
 
 @mock.patch("openfe.utils.visualization._draw_molecules", autospec=True)