Repository: mosdef-hub/gmso

Issue:
Relax validation constraints and add clone method to Potentials
I noticed that while deepcopying potentials, its super inefficient that we validate the expressions again, which is unnecessary. Similar to a clone method in mbuild, it makes sense to add clone method to all our potentials, cache sympification of expressions and finally add a validation flag to potential expressions's constructor.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/mosdef-hub/gmso/pull/642?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) Report
> Merging [#642](https://codecov.io/gh/mosdef-hub/gmso/pull/642?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) (8ce1deb) into [master](https://codecov.io/gh/mosdef-hub/gmso/commit/0f5429e8e511798289b3fc332212f5936fc993f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) (0f5429e) will **decrease** coverage by `85.60%`.
> The diff coverage is `19.23%`.

> :exclamation: Current head 8ce1deb differs from pull request most recent head 8a5adf4. Consider uploading reports for the commit 8a5adf4 to get more accurate results

```diff
@@            Coverage Diff             @@
##           master    #642       +/-   ##
==========================================
- Coverage   90.49%   4.88%   -85.61%     
==========================================
  Files          56      56               
  Lines        4504    4521       +17     
==========================================
- Hits         4076     221     -3855     
- Misses        428    4300     +3872     
```


| [Impacted Files](https://codecov.io/gh/mosdef-hub/gmso/pull/642?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) | Coverage Δ | |
|---|---|---|
| [gmso/core/atom\_type.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby9jb3JlL2F0b21fdHlwZS5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [gmso/core/parametric\_potential.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby9jb3JlL3BhcmFtZXRyaWNfcG90ZW50aWFsLnB5) | `6.25% <11.11%> (-89.53%)` | :arrow_down: |
| [gmso/utils/expression.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby91dGlscy9leHByZXNzaW9uLnB5) | `17.03% <26.66%> (-75.22%)` | :arrow_down: |
| [gmso/core/bond.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby9jb3JlL2JvbmQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [gmso/formats/mol2.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby9mb3JtYXRzL21vbDIucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [gmso/core/dihedral.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby9jb3JlL2RpaGVkcmFsLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [gmso/core/improper.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby9jb3JlL2ltcHJvcGVyLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [gmso/core/bond\_type.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby9jb3JlL2JvbmRfdHlwZS5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [gmso/utils/singleton.py](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-Z21zby91dGlscy9zaW5nbGV0b24ucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [45 more](https://codecov.io/gh/mosdef-hub/gmso/pull/642/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/mosdef-hub/gmso/pull/642?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/mosdef-hub/gmso/pull/642?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub). Last update [0f5429e...8a5adf4](https://codecov.io/gh/mosdef-hub/gmso/pull/642?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub).

umesh-timalsina: /azp run
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 2a1ed4c..968378a 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml
@@ -2,6 +2,7 @@ trigger:
   branches:
     include:
     - master
+    - develop
   tags:
     include:
     - 0.*
@@ -11,6 +12,7 @@ pr:
   branches:
     include:
       - master
+      - develop
 
 schedules:
 - cron: "0 0 * * *"
diff --git a/gmso/core/atom_type.py b/gmso/core/atom_type.py
index 3a01f44..3a1b39b 100644
--- a/gmso/core/atom_type.py
+++ b/gmso/core/atom_type.py
@@ -124,6 +124,25 @@ class AtomType(ParametricPotential):
         """Return the SMARTS string of the atom_type."""
         return self.__dict__.get("definition_")
 
+    def clone(self):
+        """Clone this AtomType, faster alternative to deepcopying."""
+        return AtomType(
+            name=str(self.name),
+            expression=None,
+            parameters=None,
+            independent_variables=None,
+            potential_expression=self.potential_expression_.clone(),
+            mass=u.unyt_quantity(self.mass_.value, self.mass_.units),
+            charge=u.unyt_quantity(self.charge_.value, self.charge_.units),
+            atomclass=self.atomclass_,
+            doi=self.doi_,
+            overrides=set(o for o in self.overrides_)
+            if self.overrides_
+            else None,
+            description=self.description_,
+            definition=self.definition_,
+        )
+
     def __eq__(self, other):
         if other is self:
             return True
diff --git a/gmso/core/parametric_potential.py b/gmso/core/parametric_potential.py
index 0b264e5..efac3ef 100644
--- a/gmso/core/parametric_potential.py
+++ b/gmso/core/parametric_potential.py
@@ -1,3 +1,4 @@
+from copy import copy, deepcopy
 from typing import Any, Optional, Union
 
 import unyt as u
@@ -177,6 +178,26 @@ class ParametricPotential(AbstractPotential):
 
         return params
 
+    def clone(self):
+        """Clone this parametric potential, faster alternative to deepcopying."""
+        Creator = self.__class__
+        kwargs = {"tags": deepcopy(self.tags_)}
+        if hasattr(self, "member_classes"):
+            kwargs["member_classes"] = (
+                copy(self.member_classes) if self.member_classes else None
+            )
+
+        if hasattr(self, "member_types"):
+            kwargs["member_types"] = (
+                copy(self.member_types) if self.member_types else None
+            )
+
+        return Creator(
+            name=self.name,
+            potential_expression=self.potential_expression_.clone(),
+            **kwargs,
+        )
+
     @classmethod
     def from_template(cls, potential_template, parameters, topology=None):
         """Create a potential object from the potential_template.
diff --git a/gmso/utils/expression.py b/gmso/utils/expression.py
index bcdbf0a..ed1d640 100644
--- a/gmso/utils/expression.py
+++ b/gmso/utils/expression.py
@@ -1,5 +1,7 @@
 """Manage Potential functional expressions and variables."""
 import warnings
+from copy import deepcopy
+from functools import lru_cache
 
 import sympy
 import unyt as u
@@ -34,6 +36,9 @@ class PotentialExpression:
 
     parameters: dict, default=None
         A dictionary of parameter whose key is a string and values are parameters
+
+    verify_validity: bool, default=True
+        If true verify validity of the expression, parameters and independent variables
     """
 
     __slots__ = (
@@ -43,22 +48,38 @@ class PotentialExpression:
         "_is_parametric",
     )
 
-    def __init__(self, expression, independent_variables, parameters=None):
-        self._expression = self._validate_expression(expression)
-        self._independent_variables = self._validate_independent_variables(
-            independent_variables
+    def __init__(
+        self,
+        expression,
+        independent_variables,
+        parameters=None,
+        verify_validity=True,
+    ):
+        self._expression = (
+            self._validate_expression(expression)
+            if verify_validity
+            else expression
+        )
+        self._independent_variables = (
+            self._validate_independent_variables(independent_variables)
+            if verify_validity
+            else independent_variables
         )
         self._is_parametric = False
 
         if parameters is not None:
             self._is_parametric = True
-            self._parameters = self._validate_parameters(parameters)
-            self._verify_validity(
-                self._expression, self._independent_variables, self._parameters
+            self._parameters = (
+                self._validate_parameters(parameters)
+                if verify_validity
+                else parameters
             )
-        else:
+
+        if verify_validity:
             self._verify_validity(
-                self._expression, self.independent_variables, None
+                self._expression,
+                frozenset(self._independent_variables),
+                frozenset(self._parameters) if self._is_parametric else None,
             )
 
     @property
@@ -157,11 +178,15 @@ class PotentialExpression:
             else:
                 parameters = self._parameters
 
-            self._verify_validity(expression, independent_variables, parameters)
+            self._verify_validity(
+                expression,
+                frozenset(independent_variables),
+                frozenset(parameters),
+            )
 
             self._parameters.update(parameters)
         else:
-            self._verify_validity(expression, independent_variables)
+            self._verify_validity(expression, frozenset(independent_variables))
 
         self._expression = expression
         self._independent_variables = independent_variables
@@ -182,6 +207,7 @@ class PotentialExpression:
         return "".join(descr)
 
     @staticmethod
+    @lru_cache(maxsize=128)
     def _validate_expression(expression):
         """Check to see that an expression is a valid sympy expression."""
         if expression is None or isinstance(expression, sympy.Expr):
@@ -262,6 +288,20 @@ class PotentialExpression:
 
         return indep_vars
 
+    def clone(self):
+        """Return a clone of this potential expression, faster alternative to deepcopying."""
+        return PotentialExpression(
+            deepcopy(self._expression),
+            deepcopy(self._independent_variables),
+            {
+                k: u.unyt_quantity(v.value, v.units)
+                for k, v in self._parameters.items()
+            }
+            if self._is_parametric
+            else None,
+            verify_validity=False,
+        )
+
     @staticmethod
     def json(potential_expression):
         """Convert the provided potential expression to a json serializable dictionary."""
@@ -283,6 +323,7 @@ class PotentialExpression:
         return json_dict
 
     @staticmethod
+    @lru_cache(maxsize=128)
     def _verify_validity(
         expression, independent_variables_symbols, parameters=None
     ):
@@ -295,7 +336,7 @@ class PotentialExpression:
                     f"exist in the expression's free symbols {expression.free_symbols}"
                 )
         if parameters is not None:
-            parameter_symbols = sympy.symbols(set(parameters.keys()))
+            parameter_symbols = sympy.symbols(parameters)
             used_symbols = parameter_symbols.union(
                 independent_variables_symbols
             )
@@ -307,7 +348,7 @@ class PotentialExpression:
                 )
 
             if used_symbols != expression.free_symbols:
-                symbols = sympy.symbols(set(parameters.keys()))
+                symbols = sympy.symbols(parameters)
                 if symbols != expression.free_symbols:
                     missing_syms = (
                         expression.free_symbols


Test patch:
diff --git a/gmso/tests/test_atom_type.py b/gmso/tests/test_atom_type.py
index c9ae179..c3978cc 100644
--- a/gmso/tests/test_atom_type.py
+++ b/gmso/tests/test_atom_type.py
@@ -365,3 +365,40 @@ class TestAtomType(BaseTest):
         atype_dict = atype.dict(exclude={"potential_expression"})
         assert "potential_expression" not in atype_dict
         assert "charge" in atype_dict
+
+    def test_atom_type_clone(self):
+        top = Topology()
+        atype = AtomType(
+            name="ff_255",
+            expression="a*x+b+c*y",
+            independent_variables={"x", "y"},
+            parameters={"a": 200 * u.g, "b": 300 * u.K, "c": 400 * u.J},
+            mass=2.0 * u.g / u.mol,
+            charge=2.0 * u.elementary_charge,
+            atomclass="CX",
+            overrides={"ff_234"},
+            definition="CC-C",
+            description="Dummy Description",
+        )
+        atype_clone = atype.clone()
+
+        atom1 = Atom(name="1")
+        atom2 = Atom(name="2")
+        atom1.atom_type = atype
+        atom2.atom_type = atype_clone
+
+        top.add_site(atom1)
+        top.add_site(atom2)
+        top.update_topology()
+
+        assert len(top.atom_types) == 2
+
+        atype_dict = atype.dict(exclude={"topology", "set_ref"})
+        atype_clone_dict = atype_clone.dict(exclude={"topology", "set_ref"})
+
+        for key, value in atype_dict.items():
+            cloned = atype_clone_dict[key]
+            assert value == cloned
+            if id(value) == id(cloned):
+                assert isinstance(value, str)
+                assert isinstance(cloned, str)
diff --git a/gmso/tests/test_expression.py b/gmso/tests/test_expression.py
index 7b0a493..bfdb5cf 100644
--- a/gmso/tests/test_expression.py
+++ b/gmso/tests/test_expression.py
@@ -210,3 +210,25 @@ class TestExpression(BaseTest):
         assert expression_1.expression == expression_2.expression
         assert expression_3 != expression_2
         assert expression_1 != expression_3
+
+    def test_clone(self):
+        expr = PotentialExpression(
+            expression="a^2+2*a*b+b^2+2*theta*phi",
+            independent_variables={"theta", "phi"},
+            parameters={"a": 2.0 * u.nm, "b": 2.0 * u.rad},
+        )
+
+        expr_clone = expr.clone()
+
+        assert expr_clone.expression == expr.expression
+        assert id(expr_clone.expression) != id(expr.expression)
+
+        assert expr_clone.parameters == expr.parameters
+        assert id(expr_clone.parameters) != id(expr.parameters)
+
+        assert expr_clone.independent_variables == expr.independent_variables
+        assert id(expr_clone.independent_variables) != id(
+            expr.independent_variables
+        )
+
+        assert expr == expr_clone
diff --git a/gmso/tests/test_potential.py b/gmso/tests/test_potential.py
index ceb6889..a3f331b 100644
--- a/gmso/tests/test_potential.py
+++ b/gmso/tests/test_potential.py
@@ -3,7 +3,11 @@ import sympy
 import unyt as u
 from unyt.testing import assert_allclose_units
 
+from gmso.core.atom import Atom
+from gmso.core.bond import Bond
+from gmso.core.bond_type import BondType
 from gmso.core.parametric_potential import ParametricPotential
+from gmso.core.topology import Topology
 from gmso.exceptions import GMSOError
 from gmso.lib.potential_templates import PotentialTemplateLibrary
 from gmso.tests.base_test import BaseTest
@@ -229,3 +233,40 @@ class TestPotential(BaseTest):
         template = object()
         with pytest.raises(GMSOError):
             ParametricPotential.from_template(template, parameters=None)
+
+    def test_bondtype_clone(self):
+        top = Topology()
+        btype = BondType(
+            name="ff_255~ff_256",
+            expression="a*x+b+c*y",
+            independent_variables={"x", "y"},
+            parameters={"a": 200 * u.g, "b": 300 * u.K, "c": 400 * u.J},
+        )
+        btype_clone = btype.clone()
+
+        atom1 = Atom(name="1")
+        atom2 = Atom(name="2")
+        bond1 = Bond(connection_members=[atom1, atom2])
+
+        atom3 = Atom(name="3")
+        atom4 = Atom(name="4")
+        bond2 = Bond(connection_members=[atom3, atom4])
+
+        bond1.bond_type = btype
+        bond2.bond_type = btype_clone
+
+        top.add_connection(bond1)
+        top.add_connection(bond2)
+        top.update_topology()
+
+        assert len(top.bond_types) == 2
+
+        btype_dict = btype.dict(exclude={"topology", "set_ref"})
+        btype_clone_dict = btype_clone.dict(exclude={"topology", "set_ref"})
+
+        for key, value in btype_dict.items():
+            cloned = btype_clone_dict[key]
+            assert value == cloned
+            if id(value) == id(cloned):
+                assert isinstance(value, (str, type(None)))
+                assert isinstance(cloned, (str, type(None)))