Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method for deleting conformer(s) #1841

Open
mattwthompson opened this issue Mar 19, 2024 · 4 comments
Open

Add method for deleting conformer(s) #1841

mattwthompson opened this issue Mar 19, 2024 · 4 comments

Comments

@mattwthompson
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

j-wags commented Apr 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants