-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix scale charges, add unit tests #33
Conversation
hoomd_organics/base/system.py
Outdated
@@ -150,6 +150,11 @@ def mass(self): | |||
) | |||
return sum(mol.mass for mol in self.all_molecules) | |||
|
|||
@property | |||
def net_charge(self): | |||
if self.gmso_system: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we still need this if self.gmso_system
anymore? It's used in n_particles
and mass as well. I think with the recent changes, self.gmso_system
will always exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think we need it for the mass
attribute, since that is used by set_target_box
before self.gmso_system
is created, but we probably don't need it in the n_particles
and net_charge
attributes. I'll test it out and push more changes if so.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 89.17% 92.04% +2.86%
==========================================
Files 18 18
Lines 1303 1320 +17
==========================================
+ Hits 1162 1215 +53
+ Misses 141 105 -36
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you.
I added two comments related to cases where charge is None.
hoomd_organics/base/system.py
Outdated
net_charge = sum(charges) | ||
abs_charge = sum(abs(charges)) | ||
for site in self.gmso_system.sites: | ||
site.charge -= abs(site.charge) * (net_charge / abs_charge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need to take care of division by zero error for cases where abs_charge=0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And abs(site.charge)
would also cause problem if site.charge=None
.
I think one way to fix this would be to have a for loop in _convert_to_gmso()
that checks the site.charge
and if it's None sets to 0 by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll fix this after we merge your PR with addl. unit tests.
This PR adds the scale charges functionality to the
System
class that was previously in a separate utils files, and includes some unit tests.It works so that each negative charge is slightly increased and each positive charge is slightly decreased (all by the same amount) to achieve as close to a neutral charge as possible.