use gufe.AtomMapping instead of custom-defined type

[atravitz]

not for this PR, but do we want to use the actual gufe.AtomMapping https://gufe.openfree.energy/en/stable/concepts/included_models.html#atommapping

Originally posted by @atravitz in #120

[mikemhenry]

At first I thought this was just for typing, but gufe doesn't actually expose an atom map type https://github.com/OpenFreeEnergy/gufe/blob/main/gufe/mapping/ligandatommapping.py#L42 but now I think you mean refactoring things to use LigandAtomMapping and SmallMoleculeComponent instead of just the rdkit mols directly. While that would work, right now I don't quite see the benefit of that.

[IAlibay]

I'm not sure I follow this conversation, the AtomMapper does yield a LigandAtomMapping: https://github.com/OpenFreeEnergy/kartograf/blob/main/src/kartograf/atom_mapper.py#L900

[IAlibay]

@mikemhenry I think @atravitz here might mean that we should yield the AtomMapping class instead of LigandAtomMapping because technically that mapping can also be for ProteinComponents? I.e. this is what Ivan does for the protein mutation protocol. So we need to be able to account for it typing-wise.

[mikemhenry]

Okay so I noticed that I was getting a mypy error with AtomMapper:

gufe/visualization/mapping_visualization.py:450: error: "Component" has no attribute "to_rdkit"

which is because the AtomMapper just has Componets https://github.com/OpenFreeEnergy/gufe/blob/main/gufe/mapping/atom_mapping.py#L12-L14

However, LigandAtomMapping https://github.com/OpenFreeEnergy/gufe/blob/main/gufe/mapping/ligandatommapping.py#L24-L34 actually has SmallMoleculeComponent

So nothing should be using gufe.AtomMapping or gufe.AtomMapper directly since these are base classes that just have Components.

I do think that we have a naming issue since a LigandAtomMapping (at first glance) looks like it would work with a ProteinComponent

This is more of a gufe issue/discussion than a kartograf discussion, not sure if we should move the issue over there or not.

[atravitz]

Apologies for the confusing @mikemhenry @IAlibay - this was meant to be a small nitpick!

When I originally reviewed, I saw that kartograf had defined ATOM_MAPPING = dict[int, int] here, and given the molA, molB, mapping signature, I was thinking that we could maybe unify this with the gufe API and pass around an actual gufe object.
I agree with @mikemhenry that this is not actually useful

After looking at it more closely, I think what is a bit confusing is the use of "atom mapping" to mean two different things: (1) the gufe object AtomMapping and (2) the actual mapping it contains, a dict[int,int].

That language is all I'd propose changing for clarity, since when I'm reading code that says ATOM_MAPPING, I'm thinking "gufe AtomMapping", not dict[int,int]:

def filter_ringbreak_changes(molA: Chem.Mol, molB: Chem.Mol, mapping: ATOM_MAPPING) -> ATOM_MAPPING:

side note: we should have a conversation about ComponentMapping, AtomMapping, and LigandAtomMapping over on the gufe side, but not in relation to this PR.