Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce charge particle warnings; add static particle_charge method #1109

Merged
merged 24 commits into from
Apr 28, 2023

Conversation

chrisjonesBSU
Copy link
Contributor

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


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

@chrisjonesBSU
Copy link
Contributor Author

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

Copy link
Contributor

@CalCraven CalCraven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to do the trick. Are there any other instances where we're having the charge error pop up? I think sometimes I see it when I try to convert to gmso.

Also, now we have some failing tests, so have to spend some time to look and see whats going on there. Looks like:
mbuild/tests/test_compound.py::TestCompound::test_none_charge
mbuild/tests/test_compound.py::TestCompound::test_resnames_parmed_cg

mbuild/compound.py Show resolved Hide resolved
mbuild/compound.py Outdated Show resolved Hide resolved
@CalCraven
Copy link
Contributor

CalCraven commented Apr 27, 2023

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
Copy link
Contributor Author

chrisjonesBSU commented Apr 27, 2023

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

mbuild/tests/test_compound.py Fixed Show fixed Hide fixed
mbuild/tests/test_compound.py Fixed Show fixed Hide fixed
@codecov
Copy link

codecov bot commented Apr 27, 2023

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
@@           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 Coverage Δ
mbuild/compound.py 97.09% <100.00%> (+0.01%) ⬆️

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

Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@daico007 daico007 merged commit e79447b into mosdef-hub:main Apr 28, 2023
@chrisjonesBSU chrisjonesBSU deleted the charge_warnings branch May 1, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants