choderalab / perses

Experiments with expanded ensembles to explore chemical space
http://perses.readthedocs.io
MIT License
181 stars 51 forks source link

Eliminate SmallMoleculeTopologyProposal #94

Closed jchodera closed 8 years ago

jchodera commented 8 years ago

The only difference is that SmallMoleculeTopologyProposal adds a molecule_smiles property but I don't think this is ever used.

My understanding was that there should be an old_metadata and new_metadata that would store whatever additional information might be needed for convenience in a dict, such as the smiles strings for old and new molecules.

pgrinaway commented 8 years ago

I think having explicit properties, attributes, and parameters for things is safer than just passing around a dict whose contents cannot easily be enforced, though.

jchodera commented 8 years ago

I could see how that could help if functions or classes that took a TopologyProposal as an argument would be able to enforce that a particular subclass (e.g. SmallMoleculeTopologyProposal) was passed at compile-time, but I don't see how that helps here. Essentially any part of the framework that accepts a TopologyProposal can only assume that the basic TopologyProposal methods are implemented---if it wants to look for more information, it would have to put it in a try...except block or check if the method exists with hasattr(). This isn't actually any easier than just checking for 'property' in metadata, except it adds about a hundred lines of code for each subclass.

Maybe I'm missing something? Is there way to enforce subclass type checking? And if so, would we want that?

pgrinaway commented 8 years ago

Yeah, good point. I guess I was thinking of a SamplerState type object, which would be different. But in this case there is essentially no difference between the property and the metadata dict. At any rate, for the small molecule proposals, it's no longer necessary to pass in any metadata, since it infers the current state. So I can get rid of the SmallMoleculeTopologyProposal thing then.

pgrinaway commented 8 years ago

Being resolved by #97