Make `Interchange.topology` read-only

[mattwthompson]

cc: @Yoshanuikabundi are there other changes we've discussed to make things safer?

[Yoshanuikabundi]

I'm unclear on what exactly making Interchange.topology read-only would actually mean. If it means prefixing the topology attribute with an underscore and creating a get_topology() or to_topology() method, I'm supportive. If it means somehow wrapping the topology attribute in a descriptor or property or something that prevents mutation, I don't think it goes far enough.

Things that making it private does that making it read-only doesn't:

• enforces single-source-of-truth for everything duplicated between Interchange and Topology
  • Box vectors
  • Positions
  • Partial charges
  • Constraints apparently - just noticed this going over the Topology docs
• simplifies API surface - for example, keeps users from reaching for interchange.topology.to_file because they don't know about interchange.to_pdb
• is more pythonic, which helps minimise user surprise

Parts of my proposal that I think are important

• There should not be a public/blessed-for-user-access topology attribute
  • we should protect users from needing to understand invariants maintained by Interchange (eg, box vectors on the topology are not kept up to date)
  • Internal code that is aware of the invariants maintained by Interchange can continue to use a private _topology attribute
• There should be a blessed-for-user-access way to get a Topology from an Interchange with all the attributes updated to match the values in the interchange, even if this loses user-provided information
  • it's important to give users access to the topology so that they can still access chemical information from the Interchange
  • Updating topology info in one place provides Interchange a lot of freedom to define the invariants for the private attribute while still ensuring users get a consistent topology out
  • for example, the partial charges on the provided topology should be the charges from the interchange parameters - users who still want their original partial charges for some reason can go back to the topology they created it with
• the blessed-for-user-access way to get a Topology from an Interchange should be a method, not an attribute or property, and should not be called Interchange.topology() (instead Interchange.get_topology() or Interchange.to_topology() or Interchange.as_topology() or similar)
  • Signals to user that modifying this object won't change the Interchange - they need to modify the interchange directly, or recreate it
  • signals to user that this is a new topology object that shares a lot of chemical info with the topology used to create the interchange, not the original topology

[mattwthompson]

If it means prefixing the topology attribute with an underscore and creating a get_topology() or to_topology() method, I'm supportive.

This is what I had in mind - right now somebody could do my_interchange.topology.add_atom and break everything. Ditto for the problems of multiple sources of truth and API surface points you raise.

I agree with every point you raise later and am looking forward to iterating on this change. Lots of details to figure out