mosdef-hub / mbuild

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

Reduce charge particle warnings; add static particle_charge method #1109

Closed chrisjonesBSU closed 1 year ago

chrisjonesBSU commented 1 year ago

PR Summary:

I am getting an insane amount charge not set warnings when performing tasks like compound.visualize() or using fill_box. I'm pretty sure what was happening is conversion.py is accessing the charge property at the particle level, and warnings are being thrown each time.

This PR adds a static method for a particle charge (same thing we do for mass). This way, the charge not set warning is not raised if being called on a particle. The warning is still raised if the compound contains children, which is really where we want the warning (summing over particle charges to return a single value)

PR Checklist


chrisjonesBSU commented 1 year ago

Oops, looks like this branch contains a bunch of old commits...but it looks like everything is caught up with the main branch except for the new changes.

Here is a quick example:

comp = mb.load("C", smiles=True)

# I don't expect charge not set warnings here:
box = mb.fill_box(compound=comp, n_compounds=100, box=[2,2,2])

# I don't expect charge not set warnings here:
comp.visualize()

# I don't expect charge not set warnings here:
comp[0].charge

# I do expect charge not set warnings here:
comp.charge
box.charge
CalCraven commented 1 year ago

The test none_charge should be where you put the different functions you've mentioned and check for either catching or not catching the warnings. The second tests looks more funky, seems like the function doesn't know how to handle searching for a charge in a coarse grained compound. The issue looks like the strange call to self._contains_only_ports(). It doesn't really make sense in this context. I'd suggest using self.children is None instead. That seems to work for both the @property mass and @property charge in place of the _contains_only_ports() call, which checks to see if the particle is actually a port particle.

This test seems to show some weird behavior. If you start with a compound with mass 2, then you add a port to it, the compound changes it's nature:

def test_mass_add_port(self):
    A = mb.Compound(mass=2.0)
    print(A)
    A.add(mb.Port())
    print(A)
    assert A.mass == 2.0

Output

<Compound pos=([0. 0. 0.]), 0 bonds, id: 6743451792>
<Compound 0 particles, 0 bonds, non-periodic, id: 6743451792>

Not entirely sure what to make of it.

chrisjonesBSU commented 1 year ago

I think maybe the better fix would be adding a conditional to _contains_only_ports. Right now, it will fail for any compound that has self.children == None. I'm not sure why the coarse grain compounds have None. Maybe this could be looked at in a different PR?

The default behavior is to have an empty OrderSet:

>>> comp = mb.Compound()
>>> comp.children
OrderedSet()

Possible Fix:

    def _contains_only_ports(self):
        if self.children:
            for part in self.children:
                if not part.port_particle:
                    return False
        return True
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (08918c7) 89.50% compared to head (2ae30a8) 89.51%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1109 +/- ## ======================================= Coverage 89.50% 89.51% ======================================= Files 61 61 Lines 6305 6311 +6 ======================================= + Hits 5643 5649 +6 Misses 662 662 ``` | [Impacted Files](https://codecov.io/gh/mosdef-hub/mbuild/pull/1109?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) | Coverage Δ | | |---|---|---| | [mbuild/compound.py](https://codecov.io/gh/mosdef-hub/mbuild/pull/1109?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-bWJ1aWxkL2NvbXBvdW5kLnB5) | `97.09% <100.00%> (+0.01%)` | :arrow_up: |

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