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

Fix return statement in _parse_coulombic #744

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

chrisjonesBSU
Copy link
Contributor

This is a quick fix I noticed when using convert_hoomd.py on a topology with no charges.

_parse_coulombic() returns None if none of the sites have a charge value. This causes an error since the function is called inside of an extend method:

 763     nbonded_forces = list()
 764     nbonded_forces.extend(
 765         _parse_coulombic(
 766             top=top,
 767             nlist=nlist,
 768             scaling_factors=coulombic_scalings,
 769             resolution=pppm_kwargs["resolution"],
 770             order=pppm_kwargs["order"],
 771             r_cut=r_cut,
 772         )

With lists, passing None to extend() gives TypeError: 'NoneType' object is not iterable

Returning an empty list instead of None seems like the simplest fix. I added a unit test that should have caught this as well.

def test_zero_charges(self):
compound = mb.load("CC", smiles=True)
com_box = mb.packing.fill_box(compound, box=[5, 5, 5], n_compounds=100)
base_units = {

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable base_units is not used.
gmso/tests/test_hoomd.py Fixed Show fixed Hide fixed
gmso/tests/test_hoomd.py Fixed Show fixed Hide fixed
@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (0e0f4d6) 91.92% compared to head (60991ef) 91.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #744      +/-   ##
==========================================
+ Coverage   91.92%   91.95%   +0.02%     
==========================================
  Files          67       67              
  Lines        6815     6815              
==========================================
+ Hits         6265     6267       +2     
+ Misses        550      548       -2     
Impacted Files Coverage Δ
gmso/external/convert_hoomd.py 92.23% <100.00%> (+0.38%) ⬆️

☔ 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! Thanks for the fix!

@daico007 daico007 merged commit 25ba417 into mosdef-hub:main Jul 10, 2023
16 checks passed
@chrisjonesBSU chrisjonesBSU deleted the hoomd_no_charges branch July 10, 2023 16:17
emarinri pushed a commit to emarinri/gmso that referenced this pull request Aug 11, 2023
* Add fixed angle support
* Fix charge sign error (gmso-wide)
* Update ring identification to handle fused systems (mbuild mosdef-hub#744)
* Increase floating point accuracy
* Increase element type length and atom type length
* Correctly write atom indices for bonds/angles/dihedrals/impropers
daico007 added a commit that referenced this pull request Sep 22, 2023
* Add fixed length bonds and angles to potential templates library.

* Working to make MCF writer feature complete. Tests in progress.

* Add fixed angle support
* Fix charge sign error (gmso-wide)
* Update ring identification to handle fused systems (mbuild #744)
* Increase floating point accuracy
* Increase element type length and atom type length
* Correctly write atom indices for bonds/angles/dihedrals/impropers

* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci

* Add dimensions to Fixed Bond and Angle templates

* Update keys in OPLS dihedral parameters dict

* Modify dihedral constant indexing from k0 to k3
to k1 to k4 in the OPLSTorsionPotential template.

* Fix typed OPLS ethane test for MCF format

* Updated from old Mie Xenon test to OPLS ethane.
* Remove assertions related to n and m parameters of the Mie potential.
* Fix floating point format issues in MCF writer.

* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci

* Add typed ethane test.

This test is not complete yet. The 1/2 factor
in the angles and OPLS dihedral is not
accounted for.

* Fix code style

* Add fixture to parse mcf into sections

* Support Pydantic v1 and v2 (#752)

Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>

* Update MCF with GMSO builtin top site sorting

* Use PotentialFilters to check for unique atomtypes

* Add test to check charge neutrality

* Add test for incompatible expressions

* Test full MCF for Mie-Xe and LJ-Ar

* Check charge neutrality in each molecule test

* Add exception for not neutral system in MCF writer

* Move parsing and neutrality check as utils

* minor docstring/comment fixes, swap out simplify with symengine expand

* [pre-commit.ci] pre-commit autoupdate (#763)

updates:
- [github.com/psf/black: 23.7.0 → 23.9.1](psf/black@23.7.0...23.9.1)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Test for 0.5 factor of OPLS dihedral potential

* Test to account for 0.5 factor in harmonic angles

* Test for rigid angles using the TIP3P model

* MCF writer test with topology with 10 argon mols

* Tentative output of multiple MCF from one Topology

* Change psi to phi and consts for ethylene xml test

* Test for 0.5 factor in OPLS dihedrals

* Add cassandra test for gmso

* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci

* Add test for two atom fragment

* Test for molecule with one ring

* Change nitrogen test to ethane ua

* Add ethane rigid reference xml

* Use Fourier dihedrals as MCF standard dihedrals.

* Fix dihedral type output fourier to opls

* Match MCF GMSO Angle header to mb Angle header

* Add a test comparing gmso and mbuild mcf writers

* Use the Fourier converter for ethylene dihedrals

* Write atom type masses instead of atom masses to account for UA beads

* Output correct case for dihedral styles in MCFs

* Run an energy calculation to compare GMSO and mBuild MCF writers

* Update gmso/tests/test_mcf.py

* Update gmso/tests/test_mcf.py

---------

Co-authored-by: Ryan S. DeFever <r.s.defever@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Matt Thompson <mattwthompson@protonmail.com>
Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
Co-authored-by: Co Quach <daico007@gmail.com>
Co-authored-by: CalCraven <nicholas.c.craven@vanderbilt.edu>
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.

None yet

2 participants