Fix 673

release-notes/next-release.md

@@ -10,6 +10,7 @@
 of SBO:0000633 (see https://sourceforge.net/p/sbo/term-request/113/)
 * Correct reading and writing of subsystem in mat.
 * General cleanup of code in mat.py
+* Fixed issue 673 (problem when copying reaction from a different model)
 
 ## Other
 

src/cobra/core/gene.py

@@ -18,7 +18,7 @@
 from ast import parse as ast_parse
 from copy import deepcopy
 from keyword import kwlist
-from typing import FrozenSet, Iterable, Optional, Set, Tuple, Union
+from typing import TYPE_CHECKING, FrozenSet, Iterable, Optional, Set, Tuple, Union
 from warnings import warn
 
 import sympy.logic.boolalg as spl
@@ -30,6 +30,10 @@
 from cobra.util.util import format_long_string
 
 
+if TYPE_CHECKING:
+    from cobra.core import Reaction
+
+
 # TODO - When https://github.com/symengine/symengine.py/issues/334 is resolved,
 #  change sympy.Symbol (above in imports) to optlang.symbolics.Symbol
 
@@ -246,10 +250,28 @@ def knock_out(self) -> None:
         context.
         """
         self.functional = False
-        for reaction in self.reactions:
+        for reaction in self.reactions:  # type: ignore
             if not reaction.functional:
                 reaction.bounds = (0, 0)
 
+    @property
+    def reactions(self) -> FrozenSet["Reaction"]:
+        """Return a frozenset of reactions.
+
+        If the gene is present in a model, calculates this set by querying
+        the model reactions for this gene.
+        If not, returns the _reaction field, see cobra.Species.
+
+        Returns
+        -------
+        FrozenSet
+            A frozenset that includes the reactions of the gene.
+        """
+        if self.model:
+            return frozenset(self.model.reactions.query(lambda x: self in x, "genes"))
+        else:
+            return frozenset(self._reaction)
+
     def _repr_html_(self):
         return f"""
         <table>

src/cobra/core/metabolite.py

@@ -1,7 +1,7 @@
 """Define the Metabolite class."""
 
 import re
-from typing import TYPE_CHECKING, Dict, Optional, Union
+from typing import TYPE_CHECKING, Dict, FrozenSet, Optional, Union
 from warnings import warn
 
 from ..exceptions import OptimizationError
@@ -15,7 +15,7 @@
     from optlang.interface import Container
     from pandas import DataFrame
 
-    from cobra.core import Solution
+    from cobra.core import Reaction, Solution
     from cobra.summary.metabolite_summary import MetaboliteSummary
 
 # Numbers are not required because of the |(?=[A-Z])? block. See the
@@ -276,6 +276,26 @@ def remove_from_model(self, destructive: bool = False) -> None:
         """
         self._model.remove_metabolites(self, destructive)
 
+    @property
+    def reactions(self) -> FrozenSet["Reaction"]:
+        """Return a frozenset of reactions.
+
+        If the metabolite is present in a model, calculates this set by querying
+        the model reactions for this metabolite.
+        If not, returns the _reaction field, see cobra.Species.
+
+        Returns
+        -------
+        FrozenSet
+            A frozenset that includes the reactions of the metabolite.
+        """
+        if self.model:
+            return frozenset(
+                self.model.reactions.query(lambda x: self in x, "metabolites")
+            )
+        else:
+            return frozenset(self._reaction)
+
     def summary(
         self,
         solution: Optional["Solution"] = None,

src/cobra/core/model.py

@@ -428,10 +428,11 @@ def copy(self) -> "Model":
             new_reaction._model = new
             new.reactions.append(new_reaction)
             # update awareness
-            for metabolite, stoic in reaction._metabolites.items():
-                new_met = new.metabolites.get_by_id(metabolite.id)
-                new_reaction._metabolites[new_met] = stoic
-                new_met._reaction.add(new_reaction)
+            new_metabolites = {
+                new.metabolites.get_by_id(metabolite.id): stoic
+                for metabolite, stoic in reaction.metabolites.items()
+            }
+            new_reaction.metabolites = new_metabolites
             new_reaction.update_genes_from_gpr()
 
         new.groups = DictList()
@@ -500,6 +501,9 @@ def add_metabolites(self, metabolite_list: Union[List, Metabolite]) -> None:
 
         # First check whether the metabolites exist in the model
         metabolite_list = [x for x in metabolite_list if x.id not in self.metabolites]
+        metabolite_list = [
+            x.copy() if x.model and x.model != self else x for x in metabolite_list
+        ]
 
         bad_ids = [
             m for m in metabolite_list if not isinstance(m.id, str) or len(m.id) < 1
@@ -558,12 +562,12 @@ def remove_metabolites(
                 group.remove_members(x)
 
             if not destructive:
-                for the_reaction in list(x._reaction):  # noqa W0212
+                for the_reaction in x.reactions:
                     the_coefficient = the_reaction._metabolites[x]  # noqa W0212
                     the_reaction.subtract_metabolites({x: the_coefficient})
 
             else:
-                for x2 in list(x._reaction):  # noqa W0212
+                for x2 in x.reactions:
                     x2.remove_from_model()
 
         self.metabolites -= metabolite_list
@@ -718,8 +722,29 @@ def existing_filter(rxn: Reaction) -> bool:
                 return False
             return True
 
-        # First check whether the reactions exist in the model.
-        pruned = DictList(filter(existing_filter, reaction_list))
+        # First check whether the reactions exist in the model. Copy reactions that
+        # belong to another model, in order not to remove them from the original model.
+        exist_but_different = set()
+        other_model = ""
+        for reaction in filter(lambda x: not existing_filter(x), reaction_list):
+            if reaction != self.reactions.get_by_id(reaction.id):
+                exist_but_different.add(reaction.id)
+                other_model = other_model or reaction.model
+        if len(exist_but_different):
+            logger.warning(
+                f"The reactions {', '.join(exist_but_different)} exist"
+                f" in both {self} and {other_model}, but have different "
+                f"properties in the two mdoels. Please check to see "
+                f"which properties are the correct ones you'd like to add."
+            )
+        pruned = DictList(
+            [
+                reaction
+                if not reaction.model or reaction.model == self
+                else reaction.copy()
+                for reaction in filter(existing_filter, reaction_list)
+            ]
+        )
 
         context = get_context(self)
 
@@ -728,23 +753,17 @@ def existing_filter(rxn: Reaction) -> bool:
             reaction._model = self
             if context:
                 context(partial(setattr, reaction, "_model", None))
-            # Build a `list()` because the dict will be modified in the loop.
-            for metabolite in list(reaction.metabolites):
-                # TODO: Maybe this can happen with
-                #  Reaction.add_metabolites(combine=False)
-                # TODO: Should we add a copy of the metabolite instead?
-                if metabolite not in self.metabolites:
-                    self.add_metabolites(metabolite)
-                # A copy of the metabolite exists in the model, the reaction
-                # needs to point to the metabolite in the model.
-                else:
-                    # FIXME: Modifying 'private' attributes is horrible.
-                    stoichiometry = reaction._metabolites.pop(metabolite)
-                    model_metabolite = self.metabolites.get_by_id(metabolite.id)
-                    reaction._metabolites[model_metabolite] = stoichiometry
-                    model_metabolite._reaction.add(reaction)
-                    if context:
-                        context(partial(model_metabolite._reaction.remove, reaction))
+            mets_to_add = [
+                met
+                for met in reaction.metabolites
+                if met.id not in self.metabolites.list_attr("id")
+            ]
+            self.add_metabolites(mets_to_add)
+            new_stoich = {
+                self.metabolites.get_by_id(met.id): stoich
+                for met, stoich in reaction.metabolites.items()
+            }
+            reaction.metabolites = new_stoich
             reaction.update_genes_from_gpr()
 
         self.reactions += pruned
@@ -814,19 +833,15 @@ def remove_reactions(
 
                 for met in reaction._metabolites:
                     if reaction in met._reaction:
-                        met._reaction.remove(reaction)
-                        if context:
-                            context(partial(met._reaction.add, reaction))
-                        if remove_orphans and len(met._reaction) == 0:
+                        met.remove_reaction(reaction)
+                        if remove_orphans and len(met.reactions) == 0:
                             self.remove_metabolites(met)
 
                 for gene in reaction._genes:
                     if reaction in gene._reaction:
-                        gene._reaction.remove(reaction)
-                        if context:
-                            context(partial(gene._reaction.add, reaction))
+                        gene.remove_reaction(reaction)
 
-                        if remove_orphans and len(gene._reaction) == 0:
+                        if remove_orphans and len(gene.reactions) == 0:
                             self.genes.remove(gene)
                             if context:
                                 context(partial(self.genes.add, gene))
@@ -869,7 +884,7 @@ def existing_filter(new_group: Group) -> bool:
             """
             if new_group.id in self.groups:
                 logger.warning(
-                    f"Ignoring group '{new_group.id}'" f" since it already exists."
+                    f"Ignoring group '{new_group.id}' since it already exists."
                 )
                 return False
             return True
@@ -1257,13 +1272,13 @@ def repair(
             self.groups._generate_index()
         if rebuild_relationships:
             for met in self.metabolites:
-                met._reaction.clear()
+                met.clear_reaction()
             for gene in self.genes:
-                gene._reaction.clear()
+                gene.clear_reaction()
             for rxn in self.reactions:
                 rxn.update_genes_from_gpr()
-                for met in rxn._metabolites:
-                    met._reaction.add(rxn)
+                for met in rxn.metabolites:
+                    met.add_reaction(rxn)
 
         # point _model to self
         for dict_list in (self.reactions, self.genes, self.metabolites, self.groups):