Add packmol_args parameter to packing fucntions. Allows user to give additional commands to PACKMOL.

[chrisjonesBSU]

PR Summary:

Each of the packing/fill functions now has a packmol_args parameter, where the user can pass in a dictionary of packmol commands with their values, and these are appended to the PACKMOL_HEADER string.

Quick example; using some of the additional input commands found here

packmol_args = {'maxit': 20, 'discale': 1.2, 'movebandrandom':"", 'movefrac': 0.02}
for arg in packmol_args:
    PACKMOL_HEADER += "{} {} \n".format(arg, packmol_args[arg])

It gives the user a little bit more control when initializing a system with packing.py.

A couple of these were really helpful for the project I am currently working on, specifically movebadrandom and movefrac. I was manually editing packing.py and adding these into PACKMOL_HEADER, but it would be nice to have an easier option of changing these packmol inputs.

I haven't updated the doc strings or made unit tests yet, I wanted to get some input first.

PR Checklist

---

• Includes appropriate unit test(s)
• Appropriate docstring(s) are added/updated
• Code is (approximately) PEP8 compliant
• Issue(s) raised/addressed?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.45%. Comparing base (2bb047e) to head (e91d14d).
Report is 6 commits behind head on main.

❗ Current head e91d14d differs from pull request most recent head 5309c2b

Please upload reports for the commit 5309c2b to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #787      +/-   ##
==========================================
+ Coverage   87.32%   87.45%   +0.12%     
==========================================
  Files          62       62              
  Lines        6525     6568      +43     
==========================================
+ Hits         5698     5744      +46     
+ Misses        827      824       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mikemhenry]

Can you add an example of its use to one of the doc strings and also a unit test? not sure which extra parameter is the best to test, but probably whichever is easiest to verify.

[chrisjonesBSU]

Ok, I added doc strings and a unit test. The best way to test is to check the PACKMOL input file, which the easiest way to access is by causing an error and inspecting the log.txt file, which is how some of the current PACKMOL tests work. The test seems a little clunky, but its checking that the packmol commands were recognized by PACKMOL, and it checks that one of the commands was actually followed (nloops)

One potential issue that needs some input from others; how should it be handled when a user passes in a command for tolerance and seed, even though these are set by parameters of the fill functions (overlap and seed)? Should they override the parameter values, should they be ignored and pass a warning message to the user to specify these values using the function parameters? IMO, I think they should over ride the parameter values. I'd be interested to hear what others think.

Along the same lines are the packmol commands for filetype and output. These (xyz and filled_xyz.name) are used behind the scenes in packing.py. Since the goal of these packing functions is to return an mBuild compound, I think that if the user were to pass in options for these they should be ignored and a warning message passed. They can use .save() if they want a specific file type created. Also interested to hear what other's think. Once these are resolved I can edit the doc strings to describe the expected behavior.

[rsdefever]

One potential issue that needs some input from others; how should it be handled when a user passes in a command for tolerance and seed, even though these are set by parameters of the fill functions (overlap and seed)? Should they override the parameter values, should they be ignored and pass a warning message to the user to specify these values using the function parameters? IMO, I think they should over ride the parameter values. I'd be interested to hear what others think.

I'm no packmol expert, but I would lean towards the override + warn option, so people know they are overriding a default.

Along the same lines are the packmol commands for filetype and output. These (xyz and filled_xyz.name) are used behind the scenes in packing.py. Since the goal of these packing functions is to return an mBuild compound, I think that if the user were to pass in options for these they should be ignored and a warning message passed. They can use .save() if they want a specific file type created. Also interested to hear what other's think. Once these are resolved I can edit the doc strings to describe the expected behavior.

Ignore + warning seems reasonable to me. Or error out with a helpful message. You can have a list of supported args (or if the list of not supported args is shorter, then that might work too. How do you handle it if a user passes in an option that is not a valid packmol argument? IMO that should be a unit test as well.

[mikemhenry]

Error when args that don't make sense or will undermine how we are using packmol (like changing the intermediate file from xyz to something esle). I think warn+error when something like overlap and seed sounds nice but that does add a lot of work for parsing. If you are up for it Chirs, I think it would be better to re-factor this a bit so all the packmol settings are passed in the kwargs dictionary and we could pass in a dictionary full of defaults. That will help keep the API less hairy.

[rsdefever]

Per discussion; use a default_args dict defined by us; The user can pass any custom_args that they wish; Parse custom_args for the few disallowed values and error out; take the rest and override any default_args with custom_args.

Example:

# Combine default/custom args and override default
custom_args = {**default_args, **custom_args}

[lgtm-com]

This pull request introduces 1 alert when merging deb4343 into aefb71f - view on LGTM.com

new alerts:

• 1 for Unused local variable

[chrisjonesBSU]

Just as a quick update on this...I kind of got stuck on figuring out how to handle parsing through all of the different commands allowed by PACKMOL (there are a lot), and which ones we should raise errors and/or warnings for, which ones make sense to append to the top of the temp txt file, and which ones should be ignored.

I still think it would be nice to be able to let the user pass in any of the file-level (i.e. not atom/structure level) PACKMOL commands, but we still need to decide to what degree we are checking that the user is not breaking any PACKMOL stuff by passing in commands that don't exist, or make sense.

[CalCraven]

IMO we don't have to support ALL of the packmol options for now. Even just increasing the number of possible options would give more general support for users. A few people have suggested particular functions that would be nice to use:
#974, #769, #725 #781. But even just the ones at the top too would be useful. I suggest we decide on a list of doable ones and useful ones and just stick to that.

[chrisjonesBSU]

IMO we don't have to support ALL of the packmol options for now. Even just increasing the number of possible options would give more general support for users. A few people have suggested particular functions that would be nice to use: #974, #769, #725 #781. But even just the ones at the top too would be useful. I suggest we decide on a list of doable ones and useful ones and just stick to that.

I agree. I'll re-visit this one as well as the PR that adds fix orientation options.

[chrisjonesBSU]

I think the main thing left to do here is 1) decide if we want to make a list of allowed packmol arguments that can be used in packmol_args and 2) If so, look through the documentation and decide what they are.

Here is an example of what using packmol_args would look like

methane = mb.load("C", smiles=True)
filled = mb.fill_box(
    compound=methane,
    n_compounds=100, 
    box=[10, 10, 10],
    packmol_args={"maxit": 10000, "movebadrandom": "", "nloop": 500}
)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.45%. Comparing base (2d16145) to head (e91d14d).

❗ Current head e91d14d differs from pull request most recent head efc1c1c

Please upload reports for the commit efc1c1c to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #787      +/-   ##
==========================================
+ Coverage   85.59%   87.45%   +1.85%     
==========================================
  Files          62       62              
  Lines        6540     6568      +28     
==========================================
+ Hits         5598     5744     +146     
+ Misses        942      824     -118     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[chrisjonesBSU]

This is the failing test:

   def test_packmol_warning(self, h2o):
        with pytest.warns(UserWarning):
            mb.fill_box(h2o, n_compounds=10, box=[1, 1, 1], overlap=10)

It's not clear to me what warning it is trying to catch, or what changes I made would cause this to fail.

I kinda think we should just remove it since we have plenty of test coverage in test_packing.py and this one is very vague.

[daico007]

LGTM

[CalCraven]

I think this is good to go, let's wait to merge #1189 since that's a more general bugfix I think to these failing packmol tests using the overlap argument, and then can be merged in right after.