Merge pull request #3440 from emma58/fme-del-fix

Disallow calling `del_component` with ComponentData arguments

Author: blnicho

pyomo/contrib/fme/fourier_motzkin_elimination.py

@@ -730,12 +730,9 @@ def post_process_fme_constraints(
                 continue
             # deactivate the constraint
             projected_constraints[i].deactivate()
-            m.del_component(obj)
-            # make objective to maximize its infeasibility
-            obj = Objective(
-                expr=projected_constraints[i].body - projected_constraints[i].lower
-            )
-            m.add_component(obj_name, obj)
+            # Our constraint looks like: 0 <= a^Tx - b, so make objective to
+            # maximize its infeasibility
+            obj.expr = projected_constraints[i].body - projected_constraints[i].lower
             results = solver_factory.solve(m)
             if results.solver.termination_condition == TerminationCondition.unbounded:
                 obj_val = -float('inf')
@@ -753,7 +750,6 @@ def post_process_fme_constraints(
                 obj_val = value(obj)
             # if we couldn't make it infeasible, it's useless
             if obj_val >= tolerance:
-                m.del_component(projected_constraints[i])
                 del projected_constraints[i]
             else:
                 projected_constraints[i].activate()

pyomo/contrib/parmest/utils/model_utils.py

@@ -134,8 +134,7 @@ def convert_params_to_vars(model, param_names=None, fix_vars=False):
             v.name in param_names for v in identify_mutable_parameters(expr)
         ):
             new_expr = replace_expressions(expr=expr, substitution_map=substitution_map)
-            model.del_component(expr)
-            model.add_component(expr.name, pyo.Expression(rule=new_expr))
+            expr.expr = new_expr
 
     # Convert Params to Vars in Constraint expressions
     num_constraints = len(list(model.component_objects(pyo.Constraint, active=True)))

pyomo/core/base/block.py

@@ -34,6 +34,7 @@
 from pyomo.common.timing import ConstructionTimer
 from pyomo.core.base.component import (
     Component,
+    ComponentData,
     ActiveComponentData,
     ModelComponentFactory,
 )
@@ -1125,16 +1126,36 @@ def del_component(self, name_or_object):
         """
         Delete a component from this block.
         """
-        obj = self.component(name_or_object)
-        # FIXME: Is this necessary?  Should this raise an exception?
-        if obj is None:
-            return
-
-        # FIXME: Is this necessary?  Should this raise an exception?
-        # if name not in self._decl:
-        #    return
+        # in-lining self.component(name_or_object) so that we can add the
+        # additional check of whether or not name_or_object is a ComponentData
+        obj = None
+        if isinstance(name_or_object, str):
+            if name_or_object in self._decl:
+                obj = self._decl_order[self._decl[name_or_object]][0]
+            else:
+                # Maintaining current behavior, but perhaps this should raise an
+                # exception?
+                return
+        else:
+            try:
+                obj = name_or_object.parent_component()
+            except AttributeError:
+                # Maintaining current behavior, but perhaps this should raise an
+                # exception?
+                return
+            if obj is not name_or_object:
+                raise ValueError(
+                    "Argument '%s' to del_component is a ComponentData object. "
+                    "Please use the Python 'del' function to delete members of "
+                    "indexed Pyomo components. The del_component function can "
+                    "only be used to delete IndexedComponents and "
+                    "ScalarComponents." % name_or_object.local_name
+                )
+            if obj.parent_block() is not self:
+                return
 
         name = obj.local_name
+
         if name in self._Block_reserved_words:
             raise ValueError(
                 "Attempting to delete a reserved block component:\n\t%s" % (obj.name,)

pyomo/core/base/component.py

@@ -732,11 +732,7 @@ class ComponentData(ComponentBase):
     This is the base class for the component data used
     in Pyomo modeling components.  Subclasses of ComponentData are
     used in indexed components, and this class assumes that indexed
-    components are subclasses of IndexedComponent.  Note that
-    ComponentData instances do not store their index.  This makes
-    some operations significantly more expensive, but these are (a)
-    associated with I/O generation and (b) this cost can be managed
-    with caches.
+    components are subclasses of IndexedComponent.
 
     Constructor arguments:
         owner           The component that owns this data object

pyomo/core/tests/unit/test_block.py

@@ -50,6 +50,7 @@
     ComponentUID,
     Any,
 )
+from pyomo.common.collections import ComponentSet
 from pyomo.common.log import LoggingIntercept
 from pyomo.common.tempfiles import TempfileManager
 from pyomo.core.base.block import (
@@ -1345,6 +1346,37 @@ def test_add_del_component(self):
         self.assertFalse('x' in m.__dict__)
         self.assertIs(m.component('x'), None)
 
+    def test_del_component_data(self):
+        m = ConcreteModel()
+        self.assertFalse(m.contains_component(Var))
+        x = m.x = Var([1, 2, 3])
+        self.assertTrue(m.contains_component(Var))
+        self.assertIs(m.component('x'), x)
+        del m.x[1]
+        self.assertTrue(m.contains_component(Var))
+        self.assertTrue('x' in m.__dict__)
+        self.assertEqual(len(m.x), 2)
+        self.assertIn(m.x[2], ComponentSet(m.x.values()))
+        self.assertIn(m.x[3], ComponentSet(m.x.values()))
+
+        # This fails:
+        with self.assertRaisesRegex(
+            ValueError,
+            r"Argument 'x\[2\]' to del_component is a ComponentData object. "
+            r"Please use the Python 'del' function to delete members of "
+            r"indexed Pyomo components. The del_component function can "
+            r"only be used to delete IndexedComponents and "
+            r"ScalarComponents.",
+        ):
+            m.del_component(m.x[2])
+
+        # But we can use del
+        del m.x[2]
+        self.assertTrue(m.contains_component(Var))
+        self.assertTrue('x' in m.__dict__)
+        self.assertEqual(len(m.x), 1)
+        self.assertIn(m.x[3], ComponentSet(m.x.values()))
+
     def test_reclassify_component(self):
         m = Block()
         m.a = Var()