mosdef-hub / mbuild

A hierarchical, component based molecule builder
https://mbuild.mosdef.org
Other
171 stars 80 forks source link

Water box #1115

Closed chrisiacovella closed 1 year ago

chrisiacovella commented 1 year ago

PR Summary:

This adds a recipe to efficiently initialize a box of 3-site water molecules at ~1000 kg/m^3. This works by reading in a 4nm x 4nm x 4 nm relaxed water configuration (run in gromacs with tip3p and saved as a .gro file ) and performing any duplication/cropping of the configuration as necessary.

Since initializing water is a very common function, it seems to be a reasonable case to have a prebuilt function, as compared to calling packmol every time, since packmol: (1) will be much slower and (2) will not necessarily generate a low energy state for water.

A few features worth noting:

usage is pretty simple:

` from mbuild.lib.recipes.water_box import Water3SiteBox

new_box = mb.Box([7.0, 3.0, 6.0]) wb = Water3SiteBox(box=new_box) `

This also addresses issue #1116 where unbounded sites in the compound got added twice after calling condense. The code will now check to ensure an unbonded particle is not part of a compound we have already added. This bug came up when I tried to initialize a tip4p configuration.

PR Checklist


chrisiacovella commented 1 year ago

Tests pass locally but fail on the cloud. I think it is related to using packmol; I'll revise the tests.

daico007 commented 1 year ago

Said failed test:


self = <mbuild.tests.test_water_box.TestWaterBox object at 0x7f1600a97220>
ethane = <Ethane 8 particles, 7 bonds, non-periodic, id: 139731837261024>

    def test_mask(self, ethane):
        box_temp = mb.Box([2.0, 2.0, 1.0])

        ethane_system = mb.fill_box(
            ethane,
            n_compounds=50,
            overlap=0.22,
            edge=0.10,
            sidemax=box_temp.Lz + 1,
            box=box_temp,
            seed=1234,
        )

        wb = Water3SiteBox(box=mb.Box([2.0, 2.0, 2.0]), mask=ethane_system)

>       assert wb.n_particles == 285
E       assert 288 == 285
E        +  where 288 = <Water3SiteBox 288 particles, 192 bonds, System box: Box: Lx=2.000000, Ly=2.000000, Lz=2.000000, xy=0.000000, xz=0.000000, yz=0.000000, , id: 139731837262032>.n_particles

/home/runner/work/mbuild/mbuild/mbuild/tests/test_water_box.py:67: AssertionError
daico007 commented 1 year ago

From the documentation about the mask option, this test will inherently flaky. I think we determined that PACKMOL on different machine (even if they are the same version), will not generate the exact final configurations.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 96.22% and project coverage change: +0.25 :tada:

Comparison is base (b4d2082) 86.89% compared to head (5248f18) 87.15%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1115 +/- ## ========================================== + Coverage 86.89% 87.15% +0.25% ========================================== Files 61 62 +1 Lines 6319 6422 +103 ========================================== + Hits 5491 5597 +106 + Misses 828 825 -3 ``` | [Impacted Files](https://app.codecov.io/gh/mosdef-hub/mbuild/pull/1115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) | Coverage Δ | | |---|---|---| | [mbuild/lib/recipes/water\_box.py](https://app.codecov.io/gh/mosdef-hub/mbuild/pull/1115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-bWJ1aWxkL2xpYi9yZWNpcGVzL3dhdGVyX2JveC5weQ==) | `95.78% <95.78%> (ø)` | | | [mbuild/compound.py](https://app.codecov.io/gh/mosdef-hub/mbuild/pull/1115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-bWJ1aWxkL2NvbXBvdW5kLnB5) | `97.11% <100.00%> (+0.02%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/mosdef-hub/mbuild/pull/1115/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

chrisiacovella commented 1 year ago

Putting a note here in case we need to reference it later. Mamba started pulling protobuf 4.22.5 (rather than 4.21.12). 4.22.5 won't import properly unless you export PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python in the shell (telling it to use python, not c++). As such, it appears that the problem may actually lie in libprotobuf (the associated c++ library, based on reported issues from prior versions where this has happened). Co pinned 4.21 and all tests appear to pass without needed to do the export statement.

chrisiacovella commented 1 year ago

It looks like bleeding edge tests are failing because it can't find symengine. I'm guessing this package just hasn't been included in the testing yaml file.

chrisiacovella commented 1 year ago

I included symengine in the environment_dev.yml file (confirmed it got installed by mamba), but bleeding edge tests still seems to be failing because they can't find symengine. .

daico007 commented 1 year ago

Symengine is technically a C++ package and will need its python API (python-symengine) to work. However, I don't thin we should add symengine to mbuild dev.yml since it technically a GMSO's dependency. Currently, the bleeding test is failing because symengine and python-symengine is not yet included as hard dependency of GMSO (pending the next release). I am more in favor of leaving it out and just have the bleeding test fail for now, since we are planning to release GMSO soon.

chrisiacovella commented 1 year ago

@daico007 I'll revert adding that to the dev.yml and then I think we are ready to merge.

daico007 commented 1 year ago

Sounds cool!