Abstract potential part of AtomType class into a new class

[mattwthompson]

The idea behind this is that the part of the AtomType class that deals with the actual potential will be nearly identical to how other components (ConnectionType, etc.) deal with it, so we should abstract it to a higher level and let those components inherit from it.

[ahy3nz]

Can you add some documentation to AtomType and Potential? I got slipped up that independent_vars is a set of sympy.Symbol and not set of str

Would it be better for AtomType to have a property/attribute nb_potential, and this nb_potential is a subclass of Potential? Right now AtomType is a subclass of Potential, so expression, params, ind_vars are all properties

[ahy3nz]

What's your opinion on "independent variable" or "expression" checking? At some point we need to make sure all our Potential expressions are 'canonical' (if someone wants to make a BondType, make sure that r is the independent variable, or make sure theta is the independent variable for AngleType)?

[mattwthompson]

My opinion is that this class should have no knowledge that is that specific and that using libraries of common potentials (i.e. the slide in which Josh listed everything that HOOMD supports) is a better approach

…

On Sat, Mar 16, 2019, 3:52 PM Alex Yang ***@***.***> wrote: What's your opinion on "independent variable" or "expression" checking? At some point we need to make sure all our Potential expressions are 'canonical' (if someone wants to make a BondType, make sure that r is the independent variable, or make sure theta is the independent variable for AngleType)? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmattwthompson%2Ftopology%2Fpull%2F96%23issuecomment-473591881&data=02%7C01%7Cmatt.thompson%40vanderbilt.edu%7Cefd42f3d9c534df4153808d6aa513fec%7Cba5a7f39e3be4ab3b45067fa80faecad%7C0%7C0%7C636883663273189494&sdata=PDBKnV6AIOQpxR6k55rF6W0Zit5HvhjdL99woc8r2EM%3D&reserved=0>, or mute the thread <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAHkVluBiB_qPXhEh2kjmUET2Wr4gsSiyks5vXVl1gaJpZM4b31Pm&data=02%7C01%7Cmatt.thompson%40vanderbilt.edu%7Cefd42f3d9c534df4153808d6aa513fec%7Cba5a7f39e3be4ab3b45067fa80faecad%7C0%7C0%7C636883663273199498&sdata=xUugTA3dWyu2jMiU%2F9c2U8O3k4PBE%2FgXZeTycqEH%2FSE%3D&reserved=0> .

[justinGilmer]

Small changes and suggestions for the first go around.

[ahy3nz]

Would it be better for AtomType to have a property/attribute nb_potential, and this nb_potential is a subclass of Potential? Right now AtomType is a subclass of Potential, so expression, params, ind_vars are all properties

This is still a lingering question I'd like to hear other opinions about. So rather than AtomType(Potential) with all attributes of Potential,; it could be AtomType(object), with a property that is a Potential.

For example, AtomType.potential = LennardJonesPotential(Potential)

[mattwthompson]

I like the inheritance as I currently have it in this PR - I think it makes sense for those attributes to be directly attributes of an AtomType instead of one layer deeper. All of the attributes we currently store in Potential make sense to me as attributes of AtomType (and further BondType, etc.). I grant that independent_variables as an attribute here feels clunky, but it's going to have to be stored somewhere anyway.

More than anything, to me, it's too deep to try and access a sigma value by doing topology.atoms[0].atom_type.potential.parameters['sigma']. The atom type exists only to separate the functional form from the specific atom, so I see this extra layer as unnecessary.

I also don't think that the approach AtomType.potential = LennardJonesPotential(Potential) works well either way we think about this storage, since we'd want to inherit .expression and .independent_variables from a library but not .parameters. I'm not sure how best to support this behavior (my first idea is a Potential.inhereit_potential() function that assigns .expression and .independent_variables from a library of known potentials set sets the parameters to be empty or something). But I don't see how my_new_atom_type.potential.inherit_potential() would be an improvement over my_new_atom_type.inherit_potential() for this feature.

[ahy3nz]

From discussion, we'll leave the inheritance as is, AtomType(Potential), which parallels BondType(Potential), etc. So AtomTypes will carry their potential expression, parameters, independent variables. Functionality regarding Potential.inherit_potential() was not discussed, and might be out of the scope of this PR.

From me, I think this PR is ready to merge

[justinGilmer]

One or two final comments/possible requests, then I am ready to approve.