Boresch restraint search crashes on ligands with three or more fused aromatic rings (`get_aromatic_rings`: `list.remove(x): x not in list`)
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- Half a day
- Newbie friendliness
- 88/100
Research direction
Start with openfe/protocols/restraint_utils/geometry/utils.py and the get_aromatic_rings function, then run the minimal RDKit reproducer from the issue. Add the suggested regression cases and verify that fused aromatic rings produce the expected systems without crashing, while separate rings remain separate.
Written by the indexing model from the issue text.
Description
Summary
openfe.protocols.restraint_utils.geometry.utils.get_aromatic_rings raises
ValueError: list.remove(x): x not in list for any molecule with three or more fused
aromatic rings, such as anthracene, phenanthrene or carbazole.
It is called during automatic Boresch guest-atom selection, so for these ligands the
AbsoluteBindingProtocol complex-leg setup unit fails. The failure only appears once
execution has started: the protocol validates, the solvent leg runs, and the complex leg
then fails at setup. SepTopProtocol uses the same restraint search, so it is likely
affected as well; I have not run it to confirm.
Environment
- openfe 1.12.0, gufe 1.12.0, RDKit 2025.09.3, Python 3.13, Linux
- The same code is still present on
mainas of 2026-09-18
Minimal reproducer
No input files are needed:
from rdkit import Chem
from openfe.protocols.restraint_utils.geometry.utils import get_aromatic_rings
for name, smiles in {
"benzene": "c1ccccc1",
"naphthalene": "c1ccc2ccccc2c1",
"anthracene": "c1ccc2cc3ccccc3cc2c1",
"phenanthrene": "c1ccc2c(c1)ccc1ccccc12",
}.items():
try:
rings = get_aromatic_rings(Chem.MolFromSmiles(smiles))
print(f"OK {name}: {len(rings)} ring system(s)")
except ValueError as error:
print(f"CRASH {name}: {error}")
Output:
OK benzene: 1 ring system(s)
OK naphthalene: 1 ring system(s)
CRASH anthracene: list.remove(x): x not in list
CRASH phenanthrene: list.remove(x): x not in list
Traceback from an ABFE run
From openfe quickrun on an AbsoluteBindingProtocol transformation whose ligand has a
fused tricyclic aromatic core:
Error: The protocol unit 'ABFE Setup: <ligand> complex leg: repeat 0 generation 0' failed with the error message:
ValueError: list.remove(x): x not in list
File ".../openfe/protocols/openmm_afe/abfe_units.py", line 252, in _get_boresch_restraint
geom = geometry.boresch.find_boresch_restraint(
File ".../openfe/protocols/restraint_utils/geometry/boresch/geometry.py", line 246, in find_boresch_restraint
guest_anchors = find_guest_atom_candidates(
File ".../openfe/protocols/restraint_utils/geometry/boresch/guest.py", line 217, in find_guest_atom_candidates
atom_pool, rings_only = _get_guest_atom_pool(rdmol, rmsf, rmsf_cutoff)
File ".../openfe/protocols/restraint_utils/geometry/boresch/guest.py", line 157, in _get_guest_atom_pool
for ring in get_aromatic_rings(rdmol):
File ".../openfe/protocols/restraint_utils/geometry/utils.py", line 105, in get_aromatic_rings
aromatic_rings.remove(y)
ValueError: list.remove(x): x not in list
Expected behaviour
Fused aromatic rings are merged into one ring system — anthracene gives a single
14-atom system, as naphthalene already gives a single 10-atom one — and restraint
selection continues.
Cause
The merge loop iterates over combinations(aromatic_rings, 2) while removing from
aromatic_rings:
for x, y in combinations(aromatic_rings, 2):
if not x.isdisjoint(y):
x.update(y)
aromatic_rings.remove(y)
itertools.combinations takes a snapshot of the list when it is created, so it keeps
yielding pairs containing rings that have already been merged and removed. For three
linearly fused rings A–B–C:
(A, B)overlap: B is merged into A and removed.(A, C)overlap, since A now contains B's atoms: C is merged into A and removed.(B, C)still overlap, but C has already been removed, soremove(C)raises.
Any molecule where three or more aromatic rings form one connected fused system reaches
step 3. Two fused rings never do, which is why naphthalene works.
Proposed fix
Merge each ring into every existing group it touches, building connected components
without mutating the sequence being iterated:
def get_aromatic_rings(rdmol: Chem.Mol) -> list[set[int]]:
ringinfo = rdmol.GetRingInfo()
arom_idxs = get_aromatic_atom_idxs(rdmol)
aromatic_rings = [
set(ring) for ring in ringinfo.AtomRings() if all(a in arom_idxs for a in ring)
]
# Merge rings that share atoms into fused ring systems
merged: list[set[int]] = []
for ring in aromatic_rings:
overlapping = [group for group in merged if not group.isdisjoint(ring)]
for group in overlapping:
merged.remove(group)
ring |= group
merged.append(ring)
return merged
Checked against these cases, all of which give the expected grouping:
| Molecule | Ring systems | Sizes |
|---|---|---|
| benzene | 1 | 6 |
| naphthalene | 1 | 10 |
| anthracene | 1 | 14 |
| phenanthrene | 1 | 14 |
| pyrene | 1 | 16 |
| carbazole | 1 | 13 |
| biphenyl (two separate rings) | 2 | 6, 6 |
| benzene linked to naphthalene by CH₂ | 2 | 6, 10 |
A suggested regression test:
@pytest.mark.parametrize(
"smiles, sizes",
[
("c1ccc2cc3ccccc3cc2c1", [14]), # anthracene
("c1ccc2c(c1)ccc1ccccc12", [14]), # phenanthrene
("c1cc2ccc3cccc4ccc(c1)c2c34", [16]), # pyrene
("c1ccc(cc1)-c1ccccc1", [6, 6]), # biphenyl stays two systems
],
)
def test_get_aromatic_rings_fused(smiles, sizes):
rings = get_aromatic_rings(Chem.MolFromSmiles(smiles))
assert sorted(len(r) for r in rings) == sizes
Impact
Fused tricyclic and larger aromatic cores are common in drug-like molecules — carbazoles,
acridines, phenanthridines, fused quinoxalines — and none of these can currently be run
with automatic restraint selection.
I have not checked whether the unreleased user-defined Boresch restraint support (#2019)
bypasses this code path, which would give a workaround once released.
- Dominant language
- Python
- Stars
- 332
- Forks
- 56
- Avg merge
- 5d 2h
- Merged PRs (30d)
- 13
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from OpenFreeEnergy/openfe
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
OpenFreeEnergy/openfe#2190 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 64/100
OpenFreeEnergy/openfe#1942 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 65/100
OpenFreeEnergy/openfe#1293 ·
-
Difficulty 3/5 1-2 days Newbie friendliness 45/100
OpenFreeEnergy/openfe#2198 ·
-
Difficulty 3/5 1-2 days Newbie friendliness 25/100
OpenFreeEnergy/openfe#2196 ·
All issues in OpenFreeEnergy/openfe
Similar issues
-
area/auth bug comp/agent P3 platform/discord type/security
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
NousResearch/hermes-agent#117848 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
bancolombia/sentinel#23 ·
-
test md OpenCI
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
-
integration:quickjs org:external priority:backlog topic:code-interpreter topic:middleware type:feature
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
langchain-ai/deepagents#6450 ·
-
bug client
Difficulty 2/5 1-3 hours Newbie friendliness 88/100