datamol-io / datamol

Molecular Processing Made Easy.
https://docs.datamol.io
Apache License 2.0
462 stars 48 forks source link

Bug in `dm.conformer.generate()` when energies are equal #108

Closed cwognum closed 2 years ago

cwognum commented 2 years ago

Hi,

I am trying to generate a set of conformers for the DILI dataset. For some molecules, this failed with a TypeError, e.g.:

import datamol as dm
dm.conformers.generate(dm.to_mol("[Cl-].[Na+]"))
TypeError: '<' not supported between instances of 'Conformer' and 'Conformer'
Expand for full traceback ``` TypeError Traceback (most recent call last) Input In [17], in () ----> 1 dm.conformers.generate(dm.to_mol("[Cl-].[Na+]")) File ~/mambaforge/envs/mood/lib/python3.9/site-packages/datamol/conformers/_conformers.py:202, in generate(mol, n_confs, use_random_coords, enforce_chirality, num_threads, rms_cutoff, clear_existing, align_conformers, minimize_energy, sort_by_energy, method, energy_iterations, warning_not_converged, random_seed, add_hs, ignore_failure, embed_params, verbose) 199 # Now we reorder conformers according to their energies, 200 # so the lowest energies conformers are first. 201 mol_clone = copy.deepcopy(mol) --> 202 ordered_conformers = [conf for _, conf in sorted(zip(energies, mol_clone.GetConformers()))] 203 mol.RemoveAllConformers() 204 [mol.AddConformer(conf, assignId=True) for conf in ordered_conformers] TypeError: '<' not supported between instances of 'Conformer' and 'Conformer' ```

I believe this happens due to the presence of equal energies, due to which a call to sorted() uses the second entry in each tuple. A simple fix could be to provide the key param to sorted() in line 202. Happy to contribute if you agree with the proposed solution!

ordered_conformers = [conf for _, conf in sorted(zip(energies, mol_clone.GetConformers()), key=lambda x: x[0])]
maclandrol commented 2 years ago

@cwognum, thanks.

Can you make a PR ?

cwognum commented 2 years ago

@maclandrol Done! Please do not hesitate to let me know if anything has to be changed.

hadim commented 2 years ago

Nice catch and thanks Cas. Sorting a tuple that way were clearly a bad idea xD