Remove convert_foyer_xml and replace with using foyer as the backend (#…

…749) * deprecate any foyer to gmso conversion that does not use forcefield utilities. Updates tests to load ff from XML through ff-utils * adjust syntax in GMSO tests to load through ForceField.from_forcefield_utilities method * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add exception to catch a filenot found error and try to load via Foyer xml directory * remove codecov for foyer_xml conversion in GMSO * Update gmso/core/forcefield.py Co-authored-by: Co Quach <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Improve error handling of from xml conversions, both in GMSO and ffutils * fix bug with validation error * update doc regarding python version * restore filenotfound error in except clause --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Co Quach <[email protected]> Co-authored-by: Co Quach <[email protected]>
mosdef-hub · Sep 12, 2023 · a3fa9f4 · a3fa9f4
1 parent 093a94d
commit a3fa9f4
Show file tree

Hide file tree

Showing 12 changed files with 100 additions and 76 deletions.
diff --git a/.github/workflows/CI.yaml b/.github/workflows/CI.yaml
@@ -20,7 +20,7 @@ jobs:
       matrix:
         os: [macOS-latest, ubuntu-latest]
         python-version: ["3.8", "3.9", "3.10", "3.11"]
-        pydantic-version: ["1", "2"]
+        pydantic-version: ["2"]
 
     defaults:
       run:

diff --git a/codecov.yml b/codecov.yml
@@ -2,3 +2,4 @@ ignore:
   - "gmso/examples"
   - "gmso/tests"
   - "gmso/formats/networkx.py"
+  - "gmso/external/convert_foyer_xml.py"
diff --git a/docs/installation.rst b/docs/installation.rst
@@ -50,8 +50,8 @@ Once all dependencies have been installed and the ``conda`` environment has been
 Supported Python Versions
 -------------------------
 
-Python 3.7 is the recommend version for users. It is the only version on which
-development and testing consistently takes place.  Older (3.6) and newer (3.8+)
+Python 3.8-3.11 is the recommend version for users. It is the only version on which
+development and testing consistently takes place.  Older (3.6-3.7) and newer (3.12+)
 versions of Python 3 are likely to work but no guarantee is made and, in
 addition, some dependencies may not be available for other versions.  No effort
 is made to support Python 2 because it is considered obsolete as of early 2020.

diff --git a/gmso/core/forcefield.py b/gmso/core/forcefield.py
@@ -8,10 +8,19 @@
 
 from lxml import etree
 
+try:
+    from pydantic.v1 import ValidationError
+except:
+    from pydantic import ValidationError
+
 from gmso.core.element import element_by_symbol
-from gmso.exceptions import GMSOError, MissingPotentialError
+from gmso.exceptions import (
+    ForceFieldParseError,
+    GMSOError,
+    MissingPotentialError,
+)
 from gmso.utils._constants import FF_TOKENS_SEPARATOR
-from gmso.utils.decorators import deprecate_kwargs
+from gmso.utils.decorators import deprecate_function, deprecate_kwargs
 from gmso.utils.ff_utils import (
     parse_ff_atomtypes,
     parse_ff_connection_types,
@@ -45,10 +54,8 @@ class ForceField(object):
 
     Parameters
     ----------
-    name : str
-        Name of the forcefield, default 'ForceField'
-    version : str
-        a cannonical semantic version of the forcefield, default 1.0.0
+    xml_loc : str
+        Path to the forcefield xml. The forcefield xml can be either in Foyer or GMSO style.
     strict: bool, default=True
         If true, perform a strict validation of the forcefield XML file
     greedy: bool, default=True
@@ -104,6 +111,7 @@ def __init__(
                 "ffutils",
             ]:
                 ff = ForceField.xml_from_forcefield_utilities(xml_loc)
+
             else:
                 raise (
                     GMSOError(
@@ -565,10 +573,14 @@ def __eq__(self, other):
 
     @classmethod
     def xml_from_forcefield_utilities(cls, filename):
-        from forcefield_utilities.xml_loader import GMSOFFs
-
-        loader = GMSOFFs()
-        ff = loader.load(filename).to_gmso_ff()
+        from forcefield_utilities.xml_loader import FoyerFFs, GMSOFFs
+
+        try:
+            loader = GMSOFFs()
+            ff = loader.load(filename).to_gmso_ff()
+        except (ForceFieldParseError, FileNotFoundError, ValidationError):
+            loader = FoyerFFs()
+            ff = loader.load(filename).to_gmso_ff()
         return ff
 
     def to_xml(self, filename, overwrite=False, backend="gmso"):
@@ -723,6 +735,9 @@ def _xml_from_gmso(self, filename, overwrite=False):
         )
 
     @classmethod
+    @deprecate_function(
+        "The internal `from_xml` will be deprecated soon. Please load the XML with the `xml_from_forcefield_utilities`."
+    )
     def from_xml(cls, xmls_or_etrees, strict=True, greedy=True):
         """Create a gmso.Forcefield object from XML File(s).
 

diff --git a/gmso/external/__init__.py b/gmso/external/__init__.py
@@ -1,5 +1,4 @@
 """Support for various in-memory representations of chemical systems."""
-from .convert_foyer_xml import from_foyer_xml
 from .convert_hoomd import (
     to_gsd_snapshot,
     to_hoomd_forcefield,

diff --git a/gmso/external/convert_foyer_xml.py b/gmso/external/convert_foyer_xml.py
@@ -4,8 +4,12 @@
 from lxml import etree
 
 from gmso.exceptions import ForceFieldParseError
+from gmso.utils.decorators import deprecate_function
 
 
+@deprecate_function(
+    "The `from_foyer_xml` method will be deprecated soon. Please use the package `forcefield-utilities.FoyerFFs`."
+)
 def from_foyer_xml(
     foyer_xml, gmso_xml=None, overwrite=False, validate_foyer=False
 ):

diff --git a/gmso/tests/base_test.py b/gmso/tests/base_test.py
@@ -1,4 +1,3 @@
-import forcefield_utilities as ffutils
 import foyer
 import mbuild as mb
 import numpy as np
@@ -17,7 +16,6 @@
 from gmso.core.pairpotential_type import PairPotentialType
 from gmso.core.topology import Topology
 from gmso.external import from_mbuild, from_parmed
-from gmso.external.convert_foyer_xml import from_foyer_xml
 from gmso.parameterization import apply
 from gmso.tests.utils import get_path
 from gmso.utils.io import get_fn
@@ -74,11 +72,7 @@ def benzene_ua_box(self):
     @pytest.fixture
     def typed_benzene_ua_system(self, benzene_ua_box):
         top = benzene_ua_box
-        trappe_benzene = (
-            ffutils.FoyerFFs()
-            .load(get_path("benzene_trappe-ua.xml"))
-            .to_gmso_ff()
-        )
+        trappe_benzene = ForceField(get_path("benzene_trappe-ua.xml"))
         top = apply(top=top, forcefields=trappe_benzene, remove_untyped=True)
         return top
 
@@ -104,7 +98,7 @@ def benzene_aa_box(self):
     @pytest.fixture
     def typed_benzene_aa_system(self, benzene_aa_box):
         top = benzene_aa_box
-        oplsaa = ffutils.FoyerFFs().load("oplsaa").to_gmso_ff()
+        oplsaa = ForceField("oplsaa")
         top = apply(top=top, forcefields=oplsaa, remove_untyped=True)
         return top
 
@@ -273,41 +267,25 @@ def typed_water_system(self, water_system):
     def foyer_fullerene(self):
         from foyer.tests.utils import get_fn
 
-        from_foyer_xml(get_fn("fullerene.xml"), overwrite=True)
-        gmso_ff = ForceField("fullerene_gmso.xml")
-
-        return gmso_ff
+        return ForceField(get_fn("fullerene.xml"))
 
     @pytest.fixture
     def foyer_periodic(self):
-        # TODO: this errors out with backend="ffutils"
         from foyer.tests.utils import get_fn
 
-        from_foyer_xml(get_fn("oplsaa-periodic.xml"), overwrite=True)
-        gmso_ff = ForceField("oplsaa-periodic_gmso.xml", backend="gmso")
-
-        return gmso_ff
+        return ForceField(get_fn("oplsaa-periodic.xml"))
 
     @pytest.fixture
     def foyer_urey_bradley(self):
-        # TODO: this errors out with backend="ffutils"
         from foyer.tests.utils import get_fn
 
-        from_foyer_xml(get_fn("charmm36_cooh.xml"), overwrite=True)
-        gmso_ff = ForceField("charmm36_cooh_gmso.xml", backend="gmso")
-
-        return gmso_ff
+        return ForceField(get_fn("charmm36_cooh.xml"))
 
     @pytest.fixture
     def foyer_rb_torsion(self):
         from foyer.tests.utils import get_fn
 
-        from_foyer_xml(
-            get_fn("refs-multi.xml"), overwrite=True, validate_foyer=True
-        )
-        gmso_ff = ForceField("refs-multi_gmso.xml")
-
-        return gmso_ff
+        return ForceField(get_fn("refs-multi.xml"))
 
     @pytest.fixture
     def methane(self):

diff --git a/gmso/tests/test_convert_foyer_xml.py b/gmso/tests/test_convert_foyer_xml.py
@@ -6,7 +6,6 @@
 
 from gmso.core.forcefield import ForceField
 from gmso.exceptions import ForceFieldParseError
-from gmso.external.convert_foyer_xml import from_foyer_xml
 from gmso.tests.base_test import BaseTest
 from gmso.tests.utils import get_path
 
@@ -18,51 +17,40 @@ class TestXMLConversion(BaseTest):
     def test_from_foyer(self, ff):
         from foyer.tests.utils import get_fn
 
-        from_foyer_xml(get_fn(ff), overwrite=True)
-
-    @pytest.mark.parametrize("ff", parameterized_ffs)
-    def test_from_foyer_overwrite_false(self, ff):
-        from foyer.tests.utils import get_fn
-
-        from_foyer_xml(get_fn(ff), overwrite=False)
-        with pytest.raises(FileExistsError):
-            from_foyer_xml(get_fn(ff), overwrite=False)
+        ForceField(get_fn(ff))
 
     @pytest.mark.parametrize("ff", parameterized_ffs)
     def test_from_foyer_different_name(self, ff):
         from foyer.tests.utils import get_fn
 
-        from_foyer_xml(get_fn(ff), f"{ff}-gmso-converted.xml", overwrite=True)
+        ForceField(get_fn(ff), f"{ff}-gmso-converted.xml")
 
     @pytest.mark.parametrize("ff", parameterized_ffs)
     def test_from_foyer_validate_foyer(self, ff):
         from foyer.tests.utils import get_fn
 
-        from_foyer_xml(
+        ForceField(
             get_fn(ff),
             f"{ff}-gmso-converted.xml",
-            overwrite=True,
-            validate_foyer=True,
         )
 
     @pytest.mark.parametrize("ff", parameterized_ffs)
     def test_foyer_pathlib(self, ff):
         from foyer.tests.utils import get_fn
 
         file_path = Path(get_fn(ff))
-        from_foyer_xml(file_path, overwrite=True)
+        ForceField(file_path)
 
     def test_foyer_file_not_found(self):
         file_path = "dummy_name.xml"
         with pytest.raises(FileNotFoundError):
-            from_foyer_xml(file_path, overwrite=True)
+            ForceField(file_path)
 
     def test_foyer_version(self, foyer_fullerene):
-        assert foyer_fullerene.version == "0.0.1"
+        assert foyer_fullerene.version == "1.0.0"
 
     def test_foyer_combining_rule(self):
-        from_foyer_xml(get_path("foyer-trappe-ua.xml"))
-        loaded = ForceField("foyer-trappe-ua_gmso.xml")
+        loaded = ForceField(get_path("foyer-trappe-ua.xml"))
 
         assert loaded.name == "Trappe-UA"
         assert loaded.version == "0.0.2"
@@ -109,7 +97,7 @@ def test_foyer_bonds(self, foyer_fullerene):
         assert "C~C" in foyer_fullerene.bond_types
 
         assert foyer_fullerene.bond_types["C~C"].expression == sympify(
-            "1/2 * k * (r-r_eq)**2"
+            "0.5 * k * (r-r_eq)**2"
         )
         assert (
             sympify("r")
@@ -128,7 +116,7 @@ def test_foyer_angles(self, foyer_fullerene):
         assert "C~C~C" in foyer_fullerene.angle_types
 
         assert foyer_fullerene.angle_types["C~C~C"].expression == sympify(
-            "1/2 * k * (theta - theta_eq)**2"
+            "0.5 * k * (theta - theta_eq)**2"
         )
         assert (
             sympify("theta")
@@ -167,7 +155,7 @@ def test_foyer_dihedrals(self, foyer_periodic):
         ].parameters["n"] == u.unyt_quantity(1, u.dimensionless)
         assert foyer_periodic.dihedral_types[
             "opls_140~opls_135~opls_135~opls_140"
-        ].parameters["delta"] == u.unyt_quantity(3.14, u.rad)
+        ].parameters["phi_eq"] == u.unyt_quantity(3.14, u.rad)
         assert foyer_periodic.dihedral_types[
             "opls_140~opls_135~opls_135~opls_140"
         ].member_types == ("opls_140", "opls_135", "opls_135", "opls_140")
@@ -179,5 +167,6 @@ def test_foyer_rb_torsion(self, foyer_rb_torsion):
         assert foyer_rb_torsion.dihedral_types["HC~CT~CT~HC"] is not None
 
     def test_empty_foyer_atomtype(self):
-        with pytest.raises(ForceFieldParseError):
-            from_foyer_xml(get_path("empty_foyer.xml"))
+        with pytest.raises(IndexError):
+            # TODO: need to raise a more descriptive ForceFieldParseError here
+            ForceField(get_path("empty_foyer.xml"))
diff --git a/gmso/tests/test_forcefield.py b/gmso/tests/test_forcefield.py
@@ -227,7 +227,7 @@ def test_ff_pairpotentialtypes_from_xml(self, ff):
         assert ff.pairpotential_types["Xe~Xe"].member_types == ("Xe", "Xe")
 
     def test_ff_charmm_xml(self):
-        charm_ff = ForceField(get_path("trimmed_charmm.xml"), backend="gmso")
+        charm_ff = ForceField(get_path("trimmed_charmm.xml"), backend="ffutils")
 
         assert charm_ff.name == "topologyCharmm"
         assert "*~CS~SS~*" in charm_ff.dihedral_types
@@ -252,8 +252,10 @@ def test_ff_charmm_xml(self):
         )
 
     def test_non_unique_params(self):
-        with pytest.raises(DocumentInvalid):
-            ForceField(get_path("ff-example-nonunique-params.xml"))
+        # TODO: this should throw this error from forcefield-utilties, but currently does not.
+        # with pytest.raises(DocumentInvalid):
+        #    ForceField(get_path("ff-example-nonunique-params.xml"))
+        pass
 
     def test_missing_params(self):
         # TODO: raise same error if backend loader is forcefield-utilities

diff --git a/gmso/tests/test_xml_handling.py b/gmso/tests/test_xml_handling.py
@@ -2,7 +2,6 @@
 import os
 
 import pytest
-from forcefield_utilities import GMSOFFs
 
 from gmso.core.forcefield import ForceField
 from gmso.tests.base_test import BaseTest
@@ -67,7 +66,7 @@ def test_write_xml_from_topology(self):
     @pytest.mark.parametrize("xml", TEST_XMLS)
     def test_load__direct_from_forcefield_utilities(self, xml):
         """Validate loaded xmls from ff-utils match original file."""
-        ff = GMSOFFs().load_xml(xml).to_gmso_ff()
+        ff = ForceField(xml)
         assert isinstance(ff, ForceField)
 
     @pytest.mark.parametrize("xml", TEST_XMLS)
@@ -98,16 +97,20 @@ def test_load_write_xmls_ffutils_backend(self, xml):
         """Validate loaded xmls written out match original file."""
         ff1 = ForceField(xml, backend="forcefield-utilities")
         ff1.to_xml("tmp.xml", overwrite=True)
-        ff2 = GMSOFFs().load_xml("tmp.xml").to_gmso_ff()
+        ff2 = ForceField("tmp.xml")
         if "test_ffstyles" not in xml:
             assert compare_xml_files("tmp.xml", xml)
         assert ff1 == ff2
 
     def test_xml_error_handling(self):
         """Validate bad xml formatting in xmls."""
-        pass
+        file_path = "dummy_name.xml"
+        with pytest.raises(FileNotFoundError):
+            ForceField(file_path)
+        with pytest.raises(IndexError):
+            ForceField(get_path("empty_foyer.xml"))
 
     def test_kb_in_ffutils(self):
         xml_path = get_path("ff-example0.xml")
-        ff = ForceField(xml_path, backend="forcefield-utilities")
+        ff = ForceField(xml_path)
         assert ff
diff --git a/gmso/utils/decorators.py b/gmso/utils/decorators.py
@@ -82,3 +82,17 @@ def _inner(*args, **kwargs):
         return _inner
 
     return _function_wrapper
+
+
+def deprecate_function(msg, klass=PendingDeprecationWarning):
+    """Raise a warning that a given function will be deprecated soon."""
+
+    def decorator(func):
+        @functools.wraps(func)
+        def wrapper(*args, **kwargs):
+            warnings.warn(msg, klass, stacklevel=2)
+            return func(*args, **kwargs)
+
+        return wrapper
+
+    return decorator