openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
301 stars 88 forks source link

Add method for deleting conformer(s) #1841

Open mattwthompson opened 3 months ago

mattwthompson commented 3 months ago

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.

lilyminium commented 3 months ago

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.

mattwthompson commented 3 months ago

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 commented 3 months ago

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

j-wags commented 2 months ago

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.