choderalab / perses

Experiments with expanded ensembles to explore chemical space
http://perses.readthedocs.io
MIT License
179 stars 51 forks source link

AtomMapping distance scoring functions not working with OFF tk 0.11 #1108

Closed richardjgowers closed 1 year ago

richardjgowers commented 2 years ago

The atom mapping scoring seems to have been broken by off tk 0.11, see here:

https://github.com/choderalab/perses/blob/main/perses/rjmc/atom_mapping.py#L1142

gives:

    return np.linalg.norm(b-a)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

args = (<Quantity([3.6608 1.1762 1.2495] /A, 'angstrom')>,), kwargs = {}
relevant_args = (<Quantity([3.6608 1.1762 1.2495] /A, 'angstrom')>,)

>   ???
E   TypeError: no implementation found for 'numpy.linalg.norm' on types that implement __array_function__: [<class 'openff.units.units.Quantity'>] 

I suspect it's that the line old_positions = atom_mapping.old_mol.conformers[0] / unit.angstroms doesn't remove units any more (unit is openmm units, whereas now the conformer arrays are openff units and these units don't cancel), so you end up with a weird "double united" Quantity array. I suspect you'll want to use @mikemhenry new ensure function or similar to cancel out units

jchodera commented 2 years ago

@mikemhenry : I'm going to have to defer to you here, since I don't understand how the new OpenFF unit system works.

mikemhenry commented 2 years ago

Looking into this, I think that unless we have a compelling reason to support both the old and new toolkit, I'd rather just update to support only the new one for perses.

ijpulidos commented 1 year ago

This should be solved with #1128 . Closing.