Closed chrisjonesBSU closed 1 year ago
Following!
Ok, turns out this wasn't really a bug in the functionality, but poor documentation and a missing check/validation step.
397 for comp, m_compounds, rotate, items_n in zip(
398 compound, n_compounds, fix_orientation, container
399 ):
In this for loop/zip, if container
is only of length 1 while compound
and n_compounds
are of length loner than 1, it only goes through this for loop once (or as long as the smallest length item in zip
)
I think we'll have to handle bounds
in the same way we do fix_orientation
343 if not isinstance(fix_orientation, (list, set)):
344 fix_orientation = [fix_orientation] * len(compound)
In the example of my original comment, I only have 1 boundary condition, that I want to fill with both methane and ethane (i.e. fill the region above a surface), so I would have to do something like:
import mbuild as mb
methane = mb.load("C", smiles=True)
ethane = mb.load("CC", smiles=True)
pack_box = mb.box.Box(lengths=[2, 2, 2], angles=[90, 90, 90])
fill_bounds = [0, 0, 1, 2, 2, 2]
region_compound = mb.fill_region(
compound=[methane, ethane],
n_compounds=[10, 10],
region=pack_box,
bounds=[[fill_bounds, fill_bounds]]
)
I think it would make sense to be able to use a single boundary condition that can be used with any length of compound
and n_compounds
, while still having the functionality to define boundaries on a molecule-by-molecule basis, which is basically how fix_orientation
works.
I'll work on a PR.
Although, we could keep it as is, and require that the user is explicit in defining packing boundaries (like my example above), then we just need a check to make sure bounds
is the same length as compound
and n_compounds
. It is convenient to only have to define the boundary condition once if you want to pack it with a mixture, but also, requiring the user to be more explicit/thoughtful in what parameters they are passing in helps avoid confusion and assumptions that cause issues later.
Good find! Yeah, I agree a smitch more documentation + better validation (so more meaningful error) would be great!
Closed by #1088
Bug summary
the
fill_region
function inpacking.py
isn't working as expected, specifically when using a list of compounds and a list ofn_compounds
Code to reproduce the behavior
I get the following error when running the
mb.fill_region
portion:fill_region
does work whencompound
andn_compounds
aren't passed in as lists larger than length 1:Software versions
python --version
)? 3.11