openff-toolkit icon indicating copy to clipboard operation
openff-toolkit copied to clipboard

Add method for deleting conformer(s)

Open mattwthompson opened this issue 1 year ago • 4 comments
trafficstars

Is your feature request related to a problem? Please describe.

There's no (public) method for deleting conformers. Molecule.generate_conformers will overwrite conformers, but in some other use cases one might want to set a molecule to have only one conformer as an Nx3 array from some other source.

Describe the solution you'd like

Molecule.clear_conformers() would be nice. There could be other methods like Molecule.delete_conformer(index=4) which pops the conformer of a particular index, but I don't really care about that.

Describe alternatives you've considered

I can just my_molecule._conformers = list() but I'm supposed to act like I don't know the "private" API exists.

Additional context

I can submit a patch for this if it's welcome ... and if I'm not oblivious to this already existing.

mattwthompson avatar Mar 19 '24 20:03 mattwthompson

This seems like a good idea. If you know Molecule.conformers is a list, you can clear it using public list methods -- Molecule.conformers.clear(). But you would need to first know that Molecule.conformers is editable (and many packages would lock this down behind tuples or similar), and secondly it can error if Molecule._conformers is None.

lilyminium avatar Mar 19 '24 20:03 lilyminium

Ah, I got it in my head that @property hid that away. Having an empty list might break some code paths that only check for None. It's tempting to suggest it always be a list instead of list | None ... but that'd be a pretty big change and is getting me off track

mattwthompson avatar Mar 19 '24 21:03 mattwthompson

I ran into just the error Lily described - it's an extra line to work around it, but the sort of thing I'd like to go in one go

mattwthompson avatar Mar 22 '24 17:03 mattwthompson

I'm running into more issues with the conformers=None vs [] issue in my QCSubmit debugging. I'm gonna try to do the minimum to fix it (trying to localize the changes in QCSubmit), but I'm becoming more amenable to the idea of only allowing one of those values. And much more interested in having a public API point for clearing conformers.

j-wags avatar Apr 22 '24 19:04 j-wags