Fix fused ring mappings

[jthorton]

An attempt at fixing #44 by enforcing that all rings an atom is present in must be present in the final mapping which should mean that fused rings are handled correctly.

[jthorton]

Some code I have been using in testing while validating the method

from kartograf import SmallMoleculeComponent
from kartograf.atom_aligner import align_mol_shape
from openff.toolkit import Molecule
from rdkit import Chem

# Preprocessing from Smiles - Here you can add your Input!
smiles = ["c1ccccc1-c2ccccc2", "C1Cc2cc(-c3ccccc3)ccc2C1"]
rdmols = [Chem.MolFromSmiles(s) for s in smiles]
rdmols = [Chem.AddHs(m, addCoords=True) for m in rdmols]
[Chem.rdDistGeom.EmbedMolecule(m, useRandomCoords=False, randomSeed = 0) for m in rdmols]

# Build Small Molecule Components
molA, molB = [SmallMoleculeComponent.from_rdkit(m) for m in rdmols]
# Align the mols first - this might not needed, depends on input.
a_molB = align_mol_shape(molB, ref_mol=molA)
from kartograf import KartografAtomMapper
# Build Kartograf Atom Mapper
mapper = KartografAtomMapper(atom_map_hydrogens=True)

# Get Mapping
kartograf_mapping = next(mapper.suggest_mappings(molA, a_molB))
print(kartograf_mapping.componentA_to_componentB) #print mapping
kartograf_mapping

with the changes in this PR we get the following mapping

compared with the mapping from the main branch

my main concern with this change is that it breaks all of the tests which check the naphtalene->benzene mapping which seems like a small regression so would we want a flag to turn this extra filtering on?

[RiesBen]

Hi @jthorton
my oppinion is: to not make this new filter a standard thing, as it is rather harsh on limiting the atom mapping regions, and I would consider this (depending on your use case) is not always necessary.
I think either adding a flag or just leave the filter in the filter collection, so Users (@JenkeScheen) that want to use this filter, can use it, is the way to go :)

@IAlibay what do you think? :)

[IAlibay]

Re: new filter - I agree, this would be the best way to do a breaking change without removing the old behaviour completely.

Re: default behaviour - this might need more discussion. I can see both viewpoints here. For the current protocols we have, we would want to avoid ring breaks because it's undefined behaviour, but I know @hannahbaumann and I had discussed that there were some cases where it might be worth having an incorrect transformation than none at all.

Main things I'd really away here are:

1. Let's make this a new filter
2. Let's discuss that we want the user experience to be

[jthorton]

Thanks for the discussion @RiesBen and @IAlibay, I agree that this does limit the number of mappings but I think this now follows the best practice advice and might be a sensible default in future. An extra filter and an option in the init to use this filter would make sense for now I'll add those. For some more context I looked into theLomapAtomMapper and PersesAtomMapper and found that lomap gave the wrong behaviour and broke the rings

and that perses gave the expected behaviour depending on if the allow_ring_breaking flag was set:
allow_ring_breaking=True

allow_ring_breaking=False

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (2112a89) to head (9d21f25).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   96.23%   96.60%   +0.36%     
==========================================
  Files          13       13              
  Lines         585      618      +33     
==========================================
+ Hits          563      597      +34     
+ Misses         22       21       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[IAlibay]

Couple of small things, otherwise lgtm!

[jthorton]

@IAlibay @hannahbaumann @RiesBen I think this is now ready for a final review, please check that the docs I added make sense to provide more information on this change.

[hannahbaumann]

Thanks @jthorton , just some small comments on the docs, otherwise lgtm!

[hannahbaumann]

Should this be True or False?

[jthorton]

Ah I was trying to make it clear what each option did but maybe I should change it to
If we should allow partially mapped fused rings (True) or not (False)?

[IAlibay]

The () would be useful visually but not a massive issue. In case Hannah is asking if it should default to False instead - for now we have to leave it to True, but we can deprecate the behaviour and aim for switching to False in the near future.

[IAlibay]

Couple of small things to address, otherwise LGTM.

[IAlibay]

The () would be useful visually but not a massive issue. In case Hannah is asking if it should default to False instead - for now we have to leave it to True, but we can deprecate the behaviour and aim for switching to False in the near future.