JuliaMolSim / Molly.jl

Molecular simulation in Julia
Other
371 stars 51 forks source link

Wrapping the interactions in a structure. #85

Open JaydevSR opened 1 year ago

JaydevSR commented 1 year ago

Right now the interaction in the library are passed down individually as keyword arguments pairwise_inters, specific_inters_list and general_inters. I have a suggenstion that maybe these can be wrapped in a structure like:

const struct Interactions{PW, SF, GN}
    pairwise_inters::PW
    specific_inters_list::SF
    general_inters::GN
end

This can be helpful in grouping together interactions that are meant for a particular simulation/system without having to worry about x3 variables that carry this information. The usefulness of this is not that much when we are talking about simulation with a single type of interactions, but becomes apparent in cases such as Hamiltonian-REMD (which I am trying to implement) where all the replicas have different force fields.

Although we can always use three different vectors to store the different variations of three interactions whenever we want to do such a thing, it seems a bit non-structured (pun intended) way of doing things, when the i'th elements are clearly meant to be used together.

Still, this will be unnecessary if such a situation is rare in practice (which I am not sure about) and also this will increase complexity a bit for simple simulations like with simple LennardJones potential where you will have to do:

inters = Interactions(pairwise_inters=(LennardJones(..), ))

...

Here a workaround can be to write simple convenience functions that set this up for the user:

LennardJonesInteraction(...) = Interactions(pairwise_inters=(LennardJones(..), ))

Please give your views on this.

jgreener64 commented 1 year ago

I am in favour of doing something like this in the long term, it makes conceptual sense to have the force field stored in once struct. It also has the advantage of allowing people to define their own force field types, since dispatch could be controlled when calling forces and potential_energy.

It would be important not to introduce too much boilerplate for users who just want to use one interaction though. It might be possible to allow the new interactions argument to System to take either an Interactions instance as above or an existing pairwise/general interaction, in which case the suggested wrapper types would not be necessary. The interaction could either be wrapped into an Interactions instance in the constructor, which is probably easiest, or could be stored as-is with dispatch being used when calling forces and potential_energy.

JaydevSR commented 1 year ago

Wrapping the interactions into an Interaction type inside the constructor seems the best. Also, the user-defined interaction type can even allow for more fields than pairwise_inters, specific_inter_lists and general_inters, where the functions forces and potential_energy will access those fields by iterating over fieldnames and using getfield for that particular type. This seems to give more freedom to anyone wanting to define custom force fields.