default mass and charge of Compound to None

[daico007]

PR Summary:

Changing the default value of Compound.charge and Compound.mass to None. The purpose is to distinct between whether the user intentionally setting the value of charge and mass or just haven't initiated those properties.

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 90.40%. Comparing base (0970f90) to head (947f87c).
Report is 153 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1047   +/-   ##
=======================================
  Coverage   90.39%   90.40%           
=======================================
  Files          64       64           
  Lines        9027     9036    +9     
=======================================
+ Hits         8160     8169    +9     
  Misses        867      867           

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

[chrisjonesBSU]

We might need a process in _validate_mas in packing.py. It currently checks for zero mass and throws a warning since the density might be affected. These lines might have an issue with None masses:

packing.py

772     found_zero_mass = False
773     total_mass = 0
774     for c, n in zip(compound, n_compounds):
775         comp_masses = [c._particle_mass(p) for p in c.particles()]
776         if 0.0 in comp_masses:
777             found_zero_mass = True
778         total_mass += np.sum(comp_masses) * n

Checking for, and handling None masses in packing.py would be very useful if someone is using mBuild to create non-atomistic compounds where mass won't be set from elemental information.

[daico007]

Good catch! I will modify the packing function to handle the case of mass = None

[lgtm-com]

This pull request introduces 1 alert when merging c409c84 into 85acbcf - view on LGTM.com

new alerts:

• 1 for Testing equality to None

[chrisjonesBSU]

I'm still getting zero mass when calling compound.mass even though it defaults to None.

import mbuild as mb

>>> comp = mb.Compound(name="A")
>>> print(comp.mass)
0
>>> print(comp._mass)
None

The Compound._particle_mass function returns zero if there is no element information; this should updated for particles with a mass of None

 363     @staticmethod
 364     def _particle_mass(particle):
 365         if particle._mass:
 366             return particle._mass
 367         else:
 368             if particle.element:
 369                 return particle.element.mass
 370             else:
 371                 return 0

Also, Compound.mass would probably fail if _particle_mass returns None

361             return sum([self._particle_mass(p) for p in self.particles()])

Maybe Compound.mass should fail and give an error if there are any NoneType masses in the compound?

[daico007]

good catch! I think we could return None and raise the warning that the mass has not been set?

[daico007]

This has been added in the latest commit.

[chrisjonesBSU]

If you're calling Compound.mass on a particle with a None mass, then we probably don't need a warning, and it can just return None. The issue arises for a container compound that has particles with mass=None

Maybe behavior along the lines of:

>>> A = mb.Compound(name="A", mass=None)
>>> B = mb.Compound(name="B", mass=1.0)
>>> C = mb.Compound([A, B])
>>> A.mass
None
>>> B.mass
1.0
>>> C.mass
Error: Contains particles with `None` mass values. The mass of a compound containing children compounds is undefined unless all children compounds have a defined mass.

Maybe the error message could be a little more clear

[lgtm-com]

This pull request introduces 1 alert when merging f448104 into 85acbcf - view on LGTM.com

new alerts:

• 1 for Testing equality to None

[lgtm-com]

This pull request introduces 1 alert when merging 2f3a42d into 85acbcf - view on LGTM.com

new alerts:

• 1 for Testing equality to None

[lgtm-com]

This pull request introduces 1 alert when merging a23ec40 into 85acbcf - view on LGTM.com

new alerts:

• 1 for Testing equality to None

[lgtm-com]

This pull request introduces 1 alert when merging 2165e38 into 85acbcf - view on LGTM.com

new alerts:

• 1 for Testing equality to None

[chrisjonesBSU]

The fixes on the mass property function look good. I guess we should probably have the same kind of process and behavior for Compound.charge and a similar unit test added.

[daico007]

similar tests for charge has been added

[lgtm-com]

This pull request introduces 1 alert when merging b878138 into 85acbcf - view on LGTM.com

new alerts:

• 1 for Testing equality to None

[lgtm-com]

This pull request introduces 1 alert when merging 7cef3dd into 85acbcf - view on LGTM.com

new alerts:

• 1 for Testing equality to None

[chrisjonesBSU]

LGTM

[lgtm-com]

This pull request introduces 1 alert when merging 947f87c into 0970f90 - view on LGTM.com

new alerts:

• 1 for Testing equality to None