search

openforcefield / openff-interchange

A project (and object) for storing, manipulating, and converting molecular mechanics data.
https://docs.openforcefield.org/projects/interchange
MIT License
66 stars 22 forks source link

Make `Interchange.topology` required? #994

Open mattwthompson opened 2 weeks ago

mattwthompson commented 2 weeks ago

Description One of the squishier details of the current designs is how many fields on the Interchange model are optional:

https://github.com/openforcefield/openff-interchange/blob/v0.3.27/openff/interchange/components/interchange.py#L140-L144

This makes sense for some fields like velocities, but how useful is an Interchange without a topology? The standard creation pathway used by ForceField.create_interchange(topology) sets the Interchange.topology attribute based on the argument to that method, so making it required should have zero impact on all or a vast majority of SMIRNOFF-centric use cases.

Are there any use cases involving Interchanges without topologies that I'm forgetting?

I have a note from @Yoshanuikabundi several months back that .from_openmm might be trickier since OpenMM topologies are strictly optional

Making topology required has the significant downside that Topology is in the openmm application API, not the library API - it's possible to use openmm without touching topologies.

but I think ~99% of use cases involve one and that could either be unsupported with minimal impact or hacked around.

Polymorphic fields have some costs - type-checkers have surprising trouble with attributes that can either be None or rich classes. In general I'd rather have the set of allowed types on a field be smaller than larger.

If this lands, it would be a breaking change as part of the 0.4.0 release.

j-wags commented 2 weeks ago

Without thinking too hard, I'm generally in favor of this. If there are use cases that don't provide a topology, the user should have to "own" the nonstandardness of whatever they're trying to do and provide a Topology-like object that has enough information to satisfy their use case. Especially since the alternative is to add a ton of complexity to Interchange to handle cases where a Topology isn't available.