search

choderalab / alchemy

Alchemical tools for OpenMM
9 stars 3 forks source link

HybridTopologyFactory #35

Closed juliebehr closed 8 years ago

juliebehr commented 8 years ago

Initial implementation

test uses perses to create 2 topologies & atom map

juliebehr commented 8 years ago

(This isn't going to build because the test uses openeye)

jchodera commented 8 years ago

Awesome!

Some questions:

jchodera commented 8 years ago

I can take care of adding the OpenEye testing machinery after this PR is merged, and we can continue development in branches of choderalab/alchemy so the OpenEye tests can run.

juliebehr commented 8 years ago

As discussed thoroughly in the issue discussing the API, dual topology refers to something entirely different from what we're doing here, but I felt within the context of what we're doing single topology is misleading, hence hybrid.

I do not believe I did make "significant" changes to the API, given that I started directly from what was discussed and only added what is required to make the topology construction possible, also following the API in examol from which 90% of this was copied. It is possible we do not need the topology to be returned, unlike examol, but we do need positions. Having both topologies as input certainly makes things much easier but I can evaluate whether it's strictly a requirement.

juliebehr commented 8 years ago

Long-term, I wouldn't think we would want alchemy to have an openeye dependence, but for right now the priority is to have this designed specifically for perses which is on a very short, high-priority timeline. relative.py could even be moved right into perses for now if that's desirable.

jchodera commented 8 years ago

As discussed thoroughly in the issue discussing the API, dual topology refers to something entirely different from what we're doing here, but I felt within the context of what we're doing single topology is misleading, hence hybrid.

OK, fair enough. I still disagree with David, but this is fine.

I do not believe I did make "significant" changes to the API, given that I started directly from what was discussed and only added what is required to make the topology construction possible, also following the API in examol from which 90% of this was copied. It is possible we do not need the topology to be returned, unlike examol, but we do need positions. Having both topologies as input certainly makes things much easier but I can evaluate whether it's strictly a requirement.

Your current API is

factory = HybridTopologyFactor(system1, system2, topology1, topology2, positions1, positions2, atom_mapping_1to2)

while we had decided (I thought?) on the API

factory = DualTopologyFactory(system1, system2, atom_mapping)

I was just surprised by the huge difference, but it sounds like you're adding a bunch of other features (creating a new Topology, as well as hybrid positions) in the mix as well. This isn't bad---it was just entirely unexpected.

relative.py could even be moved right into perses for now if that's desirable.

That may be best if it makes development easier for you, or if the resulting factory wouldn't be useful outside of perses.

juliebehr commented 8 years ago

Yes, I started with factory = DualTopologyFactory(system1, system2, atom_mapping) and quickly realized that the topologies and positions would also be required to actually make the factory functional. Part of the difference is that the initial map can have different atoms mapped to each other (e.g: the S of CYS is mapped to the O of SER), which obviously won't work for the sake of applying parameters to those atoms. The modification here is still imperfect in that it only catches different elements mapped to each other, but not necessarily different atom types of the same element. The topology is easiest to work with for identifying each atom to improve the mapping, and the positions must be returned for the output system to be usable.

jchodera commented 8 years ago

Thanks for the explanation!

This totally makes sense, but there are some API considerations if we want to use this from Yank eventually as well (which was a big driver behind the original API design).

What would you think about: *Making the Topology arguments optional, so that the atom type debugging features are available if they are specified. This would mean they are not required for the factory to function (they aren't) but would be of use in debugging if they are provided, as you describe?

If this would be a huge slowdown, we can just note these as TODOs and forge ahead, but this would get us back to the original API proposal but with some nice added optional features.

jchodera commented 8 years ago

For posterity, this was presumably closed because @juliebehr chose to migrate the code to https://github.com/choderalab/perses for more rapid development for perses-only needs.