Codecov Report

Attention: Patch coverage is 3.77358% with 153 lines in your changes missing coverage. Please review.

Project coverage is 77.06%. Comparing base (8ef8f88) to head (2d35bf3).
Report is 11 commits behind head on main.

@@             Coverage Diff             @@
##             main      #12       +/-   ##
===========================================
- Coverage   96.35%   77.06%   -19.30%     
===========================================
  Files          13       13               
  Lines         604      763      +159     
===========================================
+ Hits          582      588        +6     
- Misses         22      175      +153     

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

Hello @RiesBen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file src/kartograf/atom_mapper.py:

Line 887:9: E128 continuation line under-indented for visual indent
Line 888:9: E128 continuation line under-indented for visual indent
Line 889:9: E128 continuation line under-indented for visual indent
Line 890:9: E128 continuation line under-indented for visual indent
Line 891:9: E128 continuation line under-indented for visual indent
Line 892:9: E128 continuation line under-indented for visual indent
Line 893:9: E125 continuation line with same indent as next logical line
Line 893:9: E128 continuation line under-indented for visual indent
Line 903:9: E303 too many blank lines (2)
Line 931:80: E501 line too long (98 > 79 characters)
Line 934:80: E501 line too long (85 > 79 characters)
Line 955:5: E303 too many blank lines (2)
Line 955:80: E501 line too long (85 > 79 characters)
Line 958:42: E251 unexpected spaces around keyword / parameter equals
Line 958:44: E251 unexpected spaces around keyword / parameter equals
Line 961:9: E265 block comment should start with '# '
Line 963:18: E225 missing whitespace around operator
Line 963:18: E712 comparison to True should be 'if cond is True:' or 'if cond:'
Line 977:80: E501 line too long (128 > 79 characters)
Line 984:80: E501 line too long (81 > 79 characters)
Line 985:80: E501 line too long (93 > 79 characters)
Line 986:80: E501 line too long (87 > 79 characters)
Line 987:80: E501 line too long (92 > 79 characters)
Line 989:13: E265 block comment should start with '# '
Line 990:80: E501 line too long (95 > 79 characters)
Line 991:80: E501 line too long (103 > 79 characters)
Line 994:9: E265 block comment should start with '# '
Line 1003:25: E117 over-indented
Line 1005:46: E231 missing whitespace after ':'
Line 1021:9: E303 too many blank lines (2)
Line 1026:80: E501 line too long (96 > 79 characters)
Line 1046:80: E501 line too long (80 > 79 characters)
Line 1058:9: E303 too many blank lines (2)
Line 1061:5: E303 too many blank lines (2)
Line 1063:9: E731 do not assign a lambda expression, use a def
Line 1070:61: E261 at least two spaces before inline comment
Line 1070:62: E262 inline comment should start with '# '
Line 1072:40: E261 at least two spaces before inline comment
Line 1072:41: E262 inline comment should start with '# '
Line 1074:63: E261 at least two spaces before inline comment
Line 1074:64: E262 inline comment should start with '# '
Line 1078:80: E501 line too long (85 > 79 characters)
Line 1079:80: E501 line too long (84 > 79 characters)
Line 1095:80: E501 line too long (83 > 79 characters)
Line 1096:80: E501 line too long (127 > 79 characters)
Line 1100:80: E501 line too long (95 > 79 characters)
Line 1101:80: E501 line too long (81 > 79 characters)
Line 1104:80: E501 line too long (86 > 79 characters)
Line 1105:80: E501 line too long (91 > 79 characters)
Line 1109:80: E501 line too long (87 > 79 characters)
Line 1113:80: E501 line too long (94 > 79 characters)
Line 1116:80: E501 line too long (83 > 79 characters)
Line 1121:80: E501 line too long (96 > 79 characters)
Line 1122:80: E501 line too long (82 > 79 characters)
Line 1126:80: E501 line too long (107 > 79 characters)
Line 1131:80: E501 line too long (113 > 79 characters)
Line 1132:9: E125 continuation line with same indent as next logical line
Line 1139:80: E501 line too long (85 > 79 characters)
Line 1147:80: E501 line too long (98 > 79 characters)
Line 1157:80: E501 line too long (108 > 79 characters)

• In the file src/kartograf/utils/multistate_visualization.py:

Line 60:1: W391 blank line at end of file

• In the file src/kartograf/utils/multistate_visualization_3D.py:

Line 27:1: E303 too many blank lines (3)
Line 27:26: E231 missing whitespace after ':'
Line 28:5: E125 continuation line with same indent as next logical line
Line 54:1: E303 too many blank lines (3)
Line 56:22: E128 continuation line under-indented for visual indent
Line 56:44: E252 missing whitespace around parameter equals
Line 57:22: E128 continuation line under-indented for visual indent
Line 58:22: E128 continuation line under-indented for visual indent
Line 59:22: E124 closing bracket does not match visual indentation
Line 61:80: E501 line too long (86 > 79 characters)
Line 73:80: E501 line too long (98 > 79 characters)
Line 99:1: E302 expected 2 blank lines, found 1

Comment last updated at 2024-08-28 23:24:13 UTC

Hey @RiesBen !

So I tried using this to generate mappings for the Schindler et al. FEP benchmark, and it generally works very well. There are just some cases were the behavior isn't exactly what I would expect (just from an alchemical pathway perspective, I didn't look at your code so I don't know if these are bugs or features).

Here's the code to generate these problematic mappings:

from rdkit import Chem
from kartograf import KartografAtomMapper
from gufe.components.smallmoleculecomponent import SmallMoleculeComponent
import requests

# get ligands
url = "https://raw.githubusercontent.com/MCompChem/fep-benchmark/master/cmet/ligands.sdf"

response = requests.get(url)
with open("ligands_cmet.sdf", 'wb') as file:
    file.write(response.content)

mols = [x for x in Chem.SDMolSupplier('ligands_cmet.sdf', removeHs=False)]
components = [SmallMoleculeComponent.from_rdkit(mol) for mol in mols]

atomMapper = KartografAtomMapper()

# Get the second subset of 10 ligands from the original set
subset_components = components[10:20]
# Mapping
multi_state_mapping = atomMapper.suggest_multistate_mapping(subset_components, alternative=False, map_hydrogens=False)

This code yields the following mapping:

The alchemical transformations suggested by this mapping involve a weird ring closure/opening, which I imagine should not be allowed? I may be wrong though.

If I use the same code on another ligand set from the benchmark, url = "https://raw.githubusercontent.com/MCompChem/fep-benchmark/master/hif2a/ligands.sdf", I get

This one is clearly suspicious, as the nitrile and the sulfoxide oxygen definitely don't have the same geometry.

Wonder what you think about theses cases.

Hej @eruijsena thanks for your cool feedback! :)
I will look into the strange cases you got, I assume it might due to how the states are finally merged... let me see :)

Hi @eruijsena,
I finally had some time to improve our filter strategy and implemented a greedy mapping strategy, making everything faster! (I adapted the code example above to use the greedy approach and I added new visualization possibilities)

the new mappings give me a better impression, what do you think?

HERE we have a subset of Cmet:

2D - representation:

from kartograf.utils.multistate_visualization import visualize_multistate_mappings_2D
visualize_multistate_mappings_2D(components, multi_state_mapping)

3D - representation:

from kartograf.utils.multistate_visualization_3D import visualize_multistate_mappings_3D
visualize_multistate_mappings_3D(components, multi_state_mapping)

and here a subset of hif2a

I see again the nitrile-Sulfoxide oxygen mapping. I think for this we will need a new filter (see #35) to not map this, as the distances of our alignment make it possible to map those atoms (see the 3D):

Hey Ben.

Yes these do look better!

Would it be possible to have an explanation of what the greedy version does? I'm not sure I understand how it assigns a mapping.

For instance, if you turn the flag map_hydrogens to True in your code example with the benzenes set, I get this rather funky mapping:

Is this intended?

@eruijsena
aehm, no that is not a feature ;)
I will try to write an explanation on what the greedy algorithm does. (pls. keep in mind, this version of kartograf is still experimental, but I try to get it out of this phase :) )