feat: merge models (fix #505)

Add (reversible) `Model.merge` to handle merging two models more flexible than the
overloaded + and += operators. Deprecate those. Fix the backwards
incompatibility that added models didn't have the sum of the objectives
of the two models.

cobra/core/model.py

@@ -228,35 +228,25 @@ def set_active_bound(reaction, bound):
             set_active_bound(rxn, 0)
 
     def __add__(self, other_model):
-        """Adds two models. +
-
-        The issue of reactions being able to exists in multiple Models now
-        arises, the same for metabolites and such.  This might be a little
-        difficult as a reaction with the same name / id in two models might
-        have different coefficients for their metabolites due to pH and whatnot
-        making them different reactions.
+        """Add the content of another model to this model (+).
 
+        The model is copied as a new object, with a new model identifier,
+        and copies of all the reactions in the other model are added to this
+        model. The objective is the sum of the objective expressions for the
+        two models.
         """
-        new_model = self.copy()
-        new_reactions = deepcopy(other_model.reactions)
-        new_model.add_reactions(new_reactions)
-        new_model.id = self.id + '_' + other_model.id
-        return new_model
+        warn('use model.merge instead', DeprecationWarning)
+        return self.merge(other_model, objective='sum', inplace=False)
 
     def __iadd__(self, other_model):
-        """Adds a Model to this model +=
-
-        The issue of reactions being able to exists in multiple Models now
-        arises, the same for metabolites and such.  This might be a little
-        difficult as a reaction with the same name / id in two models might
-        have different coefficients for their metabolites due to pH and whatnot
-        making them different reactions.
+        """Incrementally add the content of another model to this model (+=).
 
+        Copies of all the reactions in the other model are added to this
+        model. The objective is the sum of the objective expressions for the
+        two models.
         """
-        new_reactions = deepcopy(other_model.reactions)
-        self.add_reactions(new_reactions)
-        self.id = self.id + '_' + other_model.id
-        return self
+        warn('use model.merge instead', DeprecationWarning)
+        return self.merge(other_model, objective='sum', inplace=True)
 
     def copy(self):
         """Provides a partial 'deepcopy' of the Model.  All of the Metabolite,
@@ -508,7 +498,6 @@ def add_reactions(self, reaction_list):
         ----------
         reaction_list : list
             A list of `cobra.Reaction` objects
-
         """
 
         try:
@@ -519,7 +508,7 @@ def add_reactions(self, reaction_list):
         # First check whether the metabolites exist in the model
         existing = [rxn for rxn in reaction_list if rxn.id in self.reactions]
         for rxn in existing:
-            LOGGER.info('skip adding reaction %s as already existing' % rxn.id)
+            LOGGER.info('skip adding reaction %s as already existing', rxn.id)
         reaction_list = [rxn for rxn in reaction_list
                          if rxn.id not in existing]
 
@@ -943,3 +932,54 @@ def __exit__(self, type, value, traceback):
         """Pop the top context manager and trigger the undo functions"""
         context = self._contexts.pop()
         context.reset()
+
+    def merge(self, right, prefix_existing=None, inplace=True,
+              objective='left'):
+        """Merge two models to create a model with the reactions from both
+        models.
+
+        Custom constraints and variables from right models are also copied
+        to left model, however note that, constraints and variables are
+        assumed to be the same if they have the same name.
+
+        right : cobra.Model
+            The model to add reactions from
+        prefix_existing : string
+            Prefix the reaction identifier in the right that already exist
+            in the left model with this string.
+        inplace : bool
+            Add reactions from right directly to left model object.
+            Otherwise, create a new model leaving the left model untouched.
+            When done within the model as context, changes to the models are
+            reverted upon exit.
+        objective : string
+            One of 'left', 'right' or 'sum' for setting the objective of the
+            resulting model to that of the corresponding model or the sum of
+            both.
+        """
+        if inplace:
+            new_model = self
+        else:
+            new_model = self.copy()
+            new_model.id = '{}_{}'.format(self.id, right.id)
+        new_reactions = deepcopy(right.reactions)
+        if prefix_existing is not None:
+            existing = new_reactions.query(
+                lambda rxn: rxn.id in self.reactions)
+            for reaction in existing:
+                reaction.id = '{}{}'.format(prefix_existing, reaction.id)
+        new_model.add_reactions(new_reactions)
+        interface = new_model.problem
+        new_vars = [interface.Variable.clone(v) for v in right.variables if
+                    v.name not in new_model.variables]
+        new_model.add_cons_vars(new_vars)
+        new_cons = [interface.Constraint.clone(c, model=new_model.solver)
+                    for c in right.constraints if
+                    c.name not in new_model.constraints]
+        new_model.add_cons_vars(new_cons, sloppy=True)
+        new_model.objective = dict(
+            left=self.objective,
+            right=right.objective,
+            sum=self.objective.expression + right.objective.expression
+        )[objective]
+        return new_model

cobra/test/conftest.py

@@ -16,6 +16,7 @@
 
 import cobra.util.solver as sutil
 from cobra.solvers import solver_dict
+from cobra import Model, Metabolite, Reaction
 
 
 def pytest_addoption(parser):
@@ -62,6 +63,19 @@ def solved_model(data_directory):
     return solution, model
 
 
+@pytest.fixture(scope="session")
+def tiny_toy_model():
+    tiny = Model("Toy Model")
+    m1 = Metabolite("M1")
+    d1 = Reaction("ex1")
+    d1.add_metabolites({m1: -1})
+    d1.upper_bound = 0
+    d1.lower_bound = -1000
+    tiny.add_reactions([d1])
+    tiny.objective = 'ex1'
+    return tiny
+
+
 @pytest.fixture(scope="function")
 def fva_results(data_directory):
     with open(join(data_directory, "textbook_fva.json"), "r") as infile:

cobra/test/test_model.py

@@ -32,7 +32,7 @@ def test_gpr(self):
         assert reaction.gene_reaction_rule == "(g1 or g2) and g3"
         assert len(reaction.genes) == 3
         # adding reaction with a GPR propagates to the model
-        model.add_reaction(reaction)
+        model.add_reactions([reaction])
         assert len(model.genes) == 3
         # ensure the gene objects are the same in the model and reaction
         reaction_gene = list(reaction.genes)[0]
@@ -134,7 +134,7 @@ def test_add_metabolite(self, model):
                        model.metabolites.get_by_id("g6p_c")] == -2
             reaction.add_metabolites({"h_c": 1})
             assert reaction._metabolites[
-                model.metabolites.get_by_id("h_c")] == 1
+                       model.metabolites.get_by_id("h_c")] == 1
             with pytest.raises(KeyError):
                 reaction.add_metabolites({"missing": 1})
 
@@ -149,10 +149,10 @@ def test_add_metabolite(self, model):
         with model:
             reaction.add_metabolites({'h2o_c': 2.5}, combine=False)
             assert reaction._metabolites[
-                model.metabolites.get_by_id("h2o_c")] == 2.5
+                       model.metabolites.get_by_id("h2o_c")] == 2.5
 
         assert reaction._metabolites[
-            model.metabolites.get_by_id("h2o_c")] == old_stoich
+                   model.metabolites.get_by_id("h2o_c")] == old_stoich
 
         # test adding to a new Reaction
         reaction = Reaction("test")
@@ -214,7 +214,7 @@ def test_build_from_string(self, model):
         assert model.metabolites.h2o_c in pgi._metabolites
         assert "foo" not in model.metabolites
         with pytest.raises(AttributeError):
-            model.metabolites.foo
+            assert model.metabolites.foo
         assert len(model.metabolites) == m
 
     def test_bounds_setter(self, model):
@@ -374,7 +374,7 @@ def test_remove_metabolite_destructive(self, model):
             assert reaction in model.reactions
 
     def test_compartments(self, model):
-        assert set(model.compartments) == set(["c", "e"])
+        assert set(model.compartments) == {"c", "e"}
 
     def test_add_reaction(self, model):
         old_reaction_count = len(model.reactions)
@@ -433,8 +433,8 @@ def test_add_reaction_context(self, model):
 
         with model:
             model.add_reaction(dummy_reaction)
-            assert model.reactions.get_by_id(dummy_reaction.id) == \
-                dummy_reaction
+            assert model.reactions.get_by_id(
+                dummy_reaction.id) == dummy_reaction
             assert len(model.reactions) == old_reaction_count + 1
             assert len(model.metabolites) == old_metabolite_count + 2
             assert dummy_metabolite_1._model == model
@@ -691,6 +691,49 @@ def test_add_reaction_orphans(self, model):
         # 'check not dangling metabolites when running Model.add_reactions
         assert len(orphan_metabolites) == 0
 
+    def test_merge_models(self, model, tiny_toy_model):
+        with model, tiny_toy_model:
+            # add some cons/vars to tiny_toy_model for testing merging
+            tiny_toy_model.add_reactions([Reaction('EX_glc__D_e')])
+            variable = tiny_toy_model.problem.Variable('foo')
+            constraint = tiny_toy_model.problem.Constraint(
+                variable, ub=0, lb=0, name='constraint')
+            tiny_toy_model.add_cons_vars([variable, constraint])
+
+            # test merging to new model
+            merged = model.merge(tiny_toy_model, inplace=False,
+                                 objective='sum', prefix_existing='tiny_')
+            assert 'ex1' in merged.reactions
+            assert 'ex1' not in model.reactions
+            assert merged.reactions.ex1.objective_coefficient == 1
+            assert merged.reactions.get_by_id(
+                'Biomass_Ecoli_core').objective_coefficient == 1
+            assert 'tiny_EX_glc__D_e' in merged.reactions
+            assert 'foo' in merged.variables
+
+            # test reversible in-place model merging
+            with model:
+                model.merge(tiny_toy_model, inplace=True, objective='left',
+                            prefix_existing='tiny_')
+                assert 'ex1' in model.reactions
+                assert 'constraint' in model.constraints
+                assert 'foo' in model.variables
+                assert 'tiny_EX_glc__D_e' in model.reactions
+                assert model.objective.expression == model.reactions.get_by_id(
+                    'Biomass_Ecoli_core').flux_expression
+            assert 'ex1' not in model.reactions
+            assert 'constraint' not in model.constraints
+            assert 'foo' not in model.variables
+            assert 'tiny_EX_glc__D_e' not in model.reactions
+
+        # test the deprecated operator overloading
+        with model:
+            merged = model + tiny_toy_model
+            assert 'ex1' in merged.reactions
+        with model:
+            model += tiny_toy_model
+            assert 'ex1' in model.reactions
+
     @pytest.mark.parametrize("solver", list(solver_dict))
     def test_change_objective_benchmark(self, model, benchmark, solver):
         atpm = model.reactions.get_by_id("ATPM")

cobra/test/test_solver_model.py

@@ -25,18 +25,6 @@
                                     reason='no cplex')]
 
 
-@pytest.fixture(scope="function")
-def tiny_toy_model():
-    model = Model("Toy Model")
-    m1 = Metabolite("M1")
-    d1 = Reaction("ex1")
-    d1.add_metabolites({m1: -1})
-    d1.upper_bound = 0
-    d1.lower_bound = -1000
-    model.add_reactions([d1])
-    return model
-
-
 @pytest.fixture(scope="function", params=solver_trials)
 def solved_model(request, model):
     model.solver = request.param

release-notes/0.6.2.md

@@ -7,12 +7,19 @@
   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).
+- Adding two models (`modela + modelb`) again results in a model with
+  the objective set to the sum of the two models objectives
+  [#505](https://github.com/opencobra/cobrapy/issues/505).
 - When adding reactions to a model, the reactions with identifiers
   identical to those in the model are
   ignored. [#511](https://github.com/opencobra/cobrapy/issues/511)
 
+## New features
+- `model.merge` can be used to merge two models, more flexibly than
+  the overloaded + and += operators.
 
 ## Deprecated features
 
-- `reaction.delete` has been deprecated in favor of
-  `reaction.remove_from_model`
+- `reaction.delete` has been deprecated in favor of `reaction.remove_from_model`
+- overloaded `+` and `+=` for `cobra.Model` are deprecated in favor of
+  `model.merge`