fix: remove_from_model was broken, deprecate deletion (#508)

* deprecating reaction.delete, fixing remove_from_model
* fix: remove unused parameter delete

cobra/core/model.py

@@ -569,8 +569,7 @@ def add_reactions(self, reaction_list):
         # from cameo ...
         self._populate_solver(reaction_list)
 
-    def remove_reactions(self, reactions, delete=True,
-                         remove_orphans=False):
+    def remove_reactions(self, reactions, remove_orphans=False):
         """Remove reactions from the model.
 
         The change is reverted upon exit when using the model as a context.
@@ -580,11 +579,6 @@ def remove_reactions(self, reactions, delete=True,
         reactions : list
             A list with reactions (`cobra.Reaction`), or their id's, to remove
 
-        delete : bool
-            Whether or not the reactions should be deleted after removal.
-            If the reactions are not deleted, those objects will be
-            recreated with new metabolite and gene objects.
-
         remove_orphans : bool
             Remove orphaned genes and metabolites from the model as well
 
@@ -596,10 +590,13 @@ def remove_reactions(self, reactions, delete=True,
         context = get_context(self)
 
         for reaction in reactions:
+
+            # Make sure the reaction is in the model
             try:
                 reaction = self.reactions[self.reactions.index(reaction)]
             except ValueError:
                 warn('%s not in %s' % (reaction, self))
+
             else:
                 forward = reaction.forward_variable
                 reverse = reaction.reverse_variable
@@ -612,27 +609,24 @@ def remove_reactions(self, reactions, delete=True,
                     context(partial(setattr, reaction, '_model', self))
                     context(partial(self.reactions.add, reaction))
 
-                for x in reaction._metabolites:
-                    if reaction in x._reaction:
-                        x._reaction.remove(reaction)
+                for met in reaction._metabolites:
+                    if reaction in met._reaction:
+                        met._reaction.remove(reaction)
                         if context:
-                            context(partial(x._reaction.add, reaction))
-                        if remove_orphans and len(x._reaction) == 0:
-                            self.remove_metabolites(x)
+                            context(partial(met._reaction.add, reaction))
+                        if remove_orphans and len(met._reaction) == 0:
+                            self.remove_metabolites(met)
 
-                for x in reaction._genes:
-                    if reaction in x._reaction:
-                        x._reaction.remove(reaction)
+                for gene in reaction._genes:
+                    if reaction in gene._reaction:
+                        gene._reaction.remove(reaction)
                         if context:
-                            context(partial(x._reaction.add, reaction))
+                            context(partial(gene._reaction.add, reaction))
 
-                        if remove_orphans and len(x._reaction) == 0:
-                            self.genes.remove(x)
+                        if remove_orphans and len(gene._reaction) == 0:
+                            self.genes.remove(gene)
                             if context:
-                                context(partial(self.genes.add, x))
-
-                reaction._metabolites = {}
-                reaction._genes = set()
+                                context(partial(self.genes.add, gene))
 
     def add_cons_vars(self, what, **kwargs):
         """Add constraints and variables to the model's mathematical problem.

cobra/core/reaction.py

@@ -524,8 +524,11 @@ def _update_awareness(self):
         for x in self._genes:
             x._reaction.add(self)
 
-    def remove_from_model(self, model=None, remove_orphans=False):
-        """Removes the reaction from the model while keeping it intact
+    def remove_from_model(self, remove_orphans=False):
+        """Removes the reaction from a model.
+
+        This removes all associations between a reaction the associated
+        model, metabolites and genes.
 
         The change is reverted upon exit when using the model as a context.
 
@@ -534,36 +537,27 @@ def remove_from_model(self, model=None, remove_orphans=False):
         remove_orphans : bool
             Remove orphaned genes and metabolites from the model as well
 
-        model : deprecated argument, must be None
         """
-
-        new_metabolites = {
-            copy(met): value for met, value in iteritems(self._metabolites)}
-
-        new_genes = {copy(i) for i in self._genes}
-
         self._model.remove_reactions([self], remove_orphans=remove_orphans)
 
-        self.add_metabolites(new_metabolites)
-        for k in new_genes:
-            self._associate_gene(k)
-
     def delete(self, remove_orphans=False):
-        """Completely delete a reaction
+        """Removes the reaction from a model.
 
         This removes all associations between a reaction the associated
-        model, metabolites and genes (unlike remove_from_model which only
-        dissociates the reaction from the model).
+        model, metabolites and genes.
 
         The change is reverted upon exit when using the model as a context.
 
+        Deprecated, use `reaction.remove_from_model` instead.
+
         Parameters
         ----------
         remove_orphans : bool
             Remove orphaned genes and metabolites from the model as well
 
         """
-        self._model.remove_reactions([self], remove_orphans=remove_orphans)
+        warn("delete is deprecated. Use reaction.remove_from_model instead")
+        self.remove_from_model(remove_orphans=remove_orphans)
 
     def __setstate__(self, state):
         """Probably not necessary to set _model as the cobra.Model that

cobra/test/test_solver_model.py

@@ -498,18 +498,23 @@ def test_add_metabolites_combine_false(self, model):
     #            ).as_coefficients_dict()
 
     def test_remove_from_model(self, model):
+        pgi = model.reactions.PGI
+        g6p = model.metabolites.g6p_c
+
         with model:
-            pgi = model.reactions.PGI
             pgi.remove_from_model()
             assert pgi.model is None
-            assert not ("PGI" in model.reactions)
-            assert not (pgi.id in model.variables)
-            assert not (pgi.reverse_id in model.variables)
-
-        assert ("PGI" in model.reactions)
-        assert (pgi.id in model.variables)
-        assert (pgi.reverse_id in model.variables)
+            assert "PGI" not in model.reactions
+            assert pgi.id not in model.variables
+            assert pgi.reverse_id not in model.variables
+            assert pgi not in g6p.reactions
+
+        assert "PGI" in model.reactions
+        assert pgi.id in model.variables
+        assert pgi.reverse_id in model.variables
         assert pgi.forward_variable.problem is model.solver
+        assert pgi in g6p.reactions
+        assert g6p in pgi.metabolites
 
     def test_delete(self, model):
         pgi = model.reactions.PGI

release-notes/0.6.2.md

@@ -0,0 +1,15 @@
+# Release notes for cobrapy 0.6.2
+
+## Fixes
+
+- Debug `model.remove_reactions` to properly work with context manager.
+  This lead to the deprecation of `reaction.delete` as this was not compatible
+  with the concept of being able to later revert the change.
+  [#506](https://github.com/opencobra/cobrapy/issues/506),
+  [#508](https://github.com/opencobra/cobrapy/pull/508).
+
+
+## Deprecated features
+
+- `reaction.delete` has been deprecated in favor of
+  `reaction.remove_from_model`