mbuild icon indicating copy to clipboard operation
mbuild copied to clipboard

Opt out of solute centering in solvate

Open mphoward opened this issue 3 years ago • 7 comments

Describe the behavior you would like added to mBuild

It would be nice to be able to opt out of centering the solute in mbuild.packing.solvate. If the solute is large, translating the center of mass may take it outside the box, leading to issues with packmol.

Describe the solution you'd like

Add an option like center=True to mbuild.packing.solvate. Then, update this template string

https://github.com/mosdef-hub/mbuild/blob/0eca09d0f613afea398003102748e5ec021b5233/mbuild/packing.py#L30-L36

so that it only uses the center option & nonzero fixed when center=True. Otherwise, only use fixed 0 0 0 0 0 0.

Describe alternatives you've considered I've tried workarounds for this, but I couldn't find something that keeps the solute from translating.

mphoward avatar Oct 29 '21 01:10 mphoward

Did this ever get fixed? Seems like a simple enough fix in the arguments to mbuild.packing.solvate! Marking this as a good first issue.

CalCraven avatar Aug 11 '22 15:08 CalCraven

I am happy to contribute this if the proposed fix above is sufficient. Or, do you want to wrap this into PR #787 that you linked?

mphoward avatar Aug 12 '22 13:08 mphoward

Hi @mphoward, it would be great if you can open a PR with the solution posted above!

daico007 avatar Aug 12 '22 14:08 daico007

Thanks, will do, probably in 1–2 weeks! (We are starting a new semester here on Tuesday.) The timing on this is excellent because I am just about ready to really want to use this feature in production. =)

mphoward avatar Aug 12 '22 14:08 mphoward

@mphoward Yeah that would be great. I think conversation has started back up on the PR mentioned above, which should be a more holistic overhaul of the PackMol functionality in mBuild. @chrisjonesBSU seems to have a few different things that he would like to wrap in all at once. I would suggest looking at the API that the PR is proposing to add using a packmol_args argument as a dictionary, and implementing the resulting affect to the packmol file that's used to generate the packing.

The two questions I have are:

  1. What would be the argument that would be intuitive here? {"center_solute":False} makes sense to me I think, as you mentioned above.
  2. Which arguments are valid for multiple different packing methods, and which are specific to each (fill_box, solvate, fill_sphere etc.)? This seems specific to just the solvate function.

CalCraven avatar Aug 12 '22 14:08 CalCraven

2. Which arguments are valid for multiple different packing methods, and which are specific to each (fill_box, solvate, fill_sphere etc.)? This seems specific to just the solvate function.

This was focused on just the solvate function. I am not sure if center makes sense (or indeed is used) in those other methods. In that sense, I am not sure whether or not this proposed change exactly fits in with the design of the other API (and welcome comments on it), as center only applies to the solute and not the other molecules in packmol. In that sense, it might be cleaner to retain only a center argument for the function itself, then accept additional packmol_args that apply to everything as in the other PR?

mphoward avatar Aug 12 '22 15:08 mphoward

I agree that in the interest of usability that we keep some of the more applicable and useful packmol options as their own parameter in the individual packing functions. Such as center_solute=True in the solvate function and edge in all of the packing functions. I know this contradicts one of the comments made in #787, but that discussion is nearly 2 years old.

This makes the useful packmol options more accessible rather than expecting the user to read through and understand all of the possible packmol parameters.

chrisjonesBSU avatar Aug 12 '22 15:08 chrisjonesBSU

Implemented in #1144

daico007 avatar Sep 26 '23 16:09 daico007