Closed mattwthompson closed 3 months ago
Thanks for the writeup. I'm in favor of allowing Topology.set_positions(None)
. Optionally Topology.clear_positions()
would be fine in addition. Happy to take a PR for these :-)
Awesome, I've opened #1827 which does this as simply as I can think of
For my own memory, this stems from https://github.com/openforcefield/openff-interchange/pull/883/files#diff-a3087103ffe505eaa0ffb73f557bffdae70a0862de3d0b802a8207167c09814fR328-R334
I decided for Topology.clear_positions()
in #1827, which has the benefit of being less ambiguous about input types and the debatable downside of adding a new method. Since I'm the person who suggested Topology.set_positions(None)
I'm also the person who can take it back
Is your feature request related to a problem? Please describe.
I'm dealing with a use case in which I want to modify a topology's positions but reset them to what they were before. The getter and setter introduced in #1368 makes this clean and first-class except for the (common, I'd argue) case in which there aren't originally positions at all.
Here is an example of how this fails and what it takes to work around it:
Running it gives the output:
Describe the solution you'd like
Just let
array=None
, which is processed internally along the lines of[molecule._conformers = None for molecule in self.molecules]
.Describe alternatives you've considered
See above and below.
Additional context
I think it's reasonable for users to expect to be able to clear positions.
Molecules
allow positions to be "un-set" via removing conformers andInterchange.positions
allowsNone
to be passed. I'm an advocate against polymorphic inputs but I think this is a case in which it's intended. To keep the API cleaner, something lineTopology.clear_positions
could be introduced that does the same thing but ensures thearray
argument cannot beNone
. I'm happy to implement either solution.Note also that
get_positions
allowsNone
as an output: