Merge pull request #777 from stairs/fix/remove-old-solver-interfaces

Remove old solver interfaces on core classes #646

cobra/core/metabolite.py

@@ -47,7 +47,6 @@ def __init__(self, id=None, formula=None, name="",
         self.compartment = compartment
         self.charge = charge
 
-        self._constraint_sense = 'E'
         self._bound = 0.
 
     def _set_id_with_model(self, value):

cobra/core/reaction.py

@@ -85,10 +85,6 @@ def __init__(self, id=None, name='', subsystem='', lower_bound=0.0,
                                       'supported. Use the model.objective '
                                       'setter')
 
-        # Used during optimization.  Indicates whether the
-        # variable is modeled as continuous, integer, or binary.
-        self.variable_kind = 'continuous'
-
         # from cameo ...
         self._lower_bound = lower_bound if lower_bound is not None else \
             CONFIGURATION.lower_bound

cobra/core/solution.py

@@ -41,19 +41,6 @@ class Solution(object):
         Contains reaction reduced costs (dual values of variables).
     shadow_prices : pandas.Series
         Contains metabolite shadow prices (dual values of constraints).
-
-    Deprecated Attributes
-    ---------------------
-    f : float
-        Use `objective_value` instead.
-    x : list
-        Use `fluxes.values` instead.
-    x_dict : pandas.Series
-        Use `fluxes` instead.
-    y : list
-        Use `reduced_costs.values` instead.
-    y_dict : pandas.Series
-        Use `reduced_costs` instead.
     """
 
     def __init__(self, objective_value, status, fluxes, reduced_costs=None,
@@ -99,16 +86,6 @@ def _repr_html_(self):
             html = '<strong><em>{}</em> solution</strong>'.format(self.status)
         return html
 
-    def __dir__(self):
-        """Hide deprecated attributes and methods from the public interface."""
-        fields = sorted(dir(type(self)) + list(self.__dict__))
-        fields.remove('f')
-        fields.remove('x')
-        fields.remove('y')
-        fields.remove('x_dict')
-        fields.remove('y_dict')
-        return fields
-
     def __getitem__(self, reaction_id):
         """
         Return the flux of a reaction.
@@ -122,50 +99,6 @@ def __getitem__(self, reaction_id):
 
     get_primal_by_id = __getitem__
 
-    @property
-    def f(self):
-        """Deprecated property for getting the objective value."""
-        warn("use solution.objective_value instead", DeprecationWarning)
-        return self.objective_value
-
-    @property
-    def x_dict(self):
-        """Deprecated property for getting fluxes."""
-        warn("use solution.fluxes instead", DeprecationWarning)
-        return self.fluxes
-
-    @x_dict.setter
-    def x_dict(self, fluxes):
-        """Deprecated property for setting fluxes."""
-        warn("let Model.optimize create a solution instance,"
-             " don't update yourself", DeprecationWarning)
-        self.fluxes = fluxes
-
-    @property
-    def x(self):
-        """Deprecated property for getting flux values."""
-        warn("use solution.fluxes.values() instead", DeprecationWarning)
-        return self.fluxes.values
-
-    @property
-    def y_dict(self):
-        """Deprecated property for getting reduced costs."""
-        warn("use solution.reduced_costs instead", DeprecationWarning)
-        return self.reduced_costs
-
-    @y_dict.setter
-    def y_dict(self, costs):
-        """Deprecated property for setting reduced costs."""
-        warn("let Model create a solution instance, don't update yourself",
-             DeprecationWarning)
-        self.reduced_costs = costs
-
-    @property
-    def y(self):
-        """Deprecated property for getting reduced cost values."""
-        warn("use solution.reduced_costs.values() instead", DeprecationWarning)
-        return self.reduced_costs.values
-
     def to_frame(self):
         """Return the fluxes and reduced costs as a data frame"""
         return DataFrame({'fluxes': self.fluxes,

cobra/flux_analysis/__init__.py

@@ -5,8 +5,7 @@
     single_reaction_deletion)
 from cobra.flux_analysis.gapfilling import gapfill
 from cobra.flux_analysis.geometric import geometric_fba
-from cobra.flux_analysis.loopless import (
-    add_loopless, construct_loopless_model, loopless_solution)
+from cobra.flux_analysis.loopless import (loopless_solution, add_loopless)
 from cobra.flux_analysis.moma import add_moma, moma
 from cobra.flux_analysis.parsimonious import pfba
 from cobra.flux_analysis.variability import (

cobra/flux_analysis/loopless.py

@@ -6,10 +6,8 @@
 
 import numpy
 from optlang.symbolics import Zero
-from six import iteritems
 
-from cobra.core import Metabolite, Reaction, get_solution
-from cobra.manipulation.modify import convert_to_irreversible
+from cobra.core import get_solution
 from cobra.util import create_stoichiometric_matrix, nullspace
 
 
@@ -241,57 +239,3 @@ def loopless_fva_iter(model, reaction, solution=False, zero_cutoff=1e-6):
             best = reaction.flux
     model.objective.direction = objective_dir
     return best
-
-
-def construct_loopless_model(cobra_model):
-    """Construct a loopless model.
-
-    This adds MILP constraints to prevent flux from proceeding in a loop, as
-    done in http://dx.doi.org/10.1016/j.bpj.2010.12.3707
-    Please see the documentation for an explanation of the algorithm.
-
-    This must be solved with an MILP capable solver.
-
-    """
-    # copy the model and make it irreversible
-    model = cobra_model.copy()
-    convert_to_irreversible(model)
-    max_ub = max(model.reactions.list_attr("upper_bound"))
-    # a dict for storing S^T
-    thermo_stoic = {"thermo_var_" + metabolite.id: {}
-                    for metabolite in model.metabolites}
-    # Slice operator is so that we don't get newly added metabolites
-    original_metabolites = model.metabolites[:]
-    for reaction in model.reactions[:]:
-        # Boundary reactions are not subjected to these constraints
-        if len(reaction._metabolites) == 1:
-            continue
-        # populate the S^T dict
-        bound_id = "thermo_bound_" + reaction.id
-        for met, stoic in iteritems(reaction._metabolites):
-            thermo_stoic["thermo_var_" + met.id][bound_id] = stoic
-        # I * 1000 > v --> I * 1000 - v > 0
-        reaction_ind = Reaction(reaction.id + "_indicator")
-        reaction_ind.variable_kind = "integer"
-        reaction_ind.upper_bound = 1
-        reaction_ub = Metabolite(reaction.id + "_ind_ub")
-        reaction_ub._constraint_sense = "G"
-        reaction.add_metabolites({reaction_ub: -1})
-        reaction_ind.add_metabolites({reaction_ub: max_ub})
-        # This adds a compensating term for 0 flux reactions, so we get
-        # S^T x - (1 - I) * 1001 < -1 which becomes
-        # S^T x < 1000 for 0 flux reactions and
-        # S^T x < -1 for reactions with nonzero flux.
-        reaction_bound = Metabolite(bound_id)
-        reaction_bound._constraint_sense = "L"
-        reaction_bound._bound = max_ub
-        reaction_ind.add_metabolites({reaction_bound: max_ub + 1})
-        model.add_reaction(reaction_ind)
-    for metabolite in original_metabolites:
-        metabolite_var = Reaction("thermo_var_" + metabolite.id)
-        metabolite_var.lower_bound = -max_ub
-        model.add_reaction(metabolite_var)
-        metabolite_var.add_metabolites(
-            {model.metabolites.get_by_id(k): v
-             for k, v in iteritems(thermo_stoic[metabolite_var.id])})
-    return model

cobra/io/dict.py

@@ -16,24 +16,22 @@
     "id", "name", "metabolites", "lower_bound", "upper_bound",
     "gene_reaction_rule"]
 _ORDERED_OPTIONAL_REACTION_KEYS = [
-    "objective_coefficient", "variable_kind", "subsystem", "notes",
+    "objective_coefficient", "subsystem", "notes",
     "annotation"]
 _OPTIONAL_REACTION_ATTRIBUTES = {
     "objective_coefficient": 0,
-    "variable_kind": "continuous",
     "subsystem": "",
     "notes": {},
     "annotation": {},
 }
 
 _REQUIRED_METABOLITE_ATTRIBUTES = ["id", "name", "compartment"]
 _ORDERED_OPTIONAL_METABOLITE_KEYS = [
-    "charge", "formula", "_bound", "_constraint_sense", "notes", "annotation"]
+    "charge", "formula", "_bound", "notes", "annotation"]
 _OPTIONAL_METABOLITE_ATTRIBUTES = {
     "charge": None,
     "formula": None,
     "_bound": 0,
-    "_constraint_sense": "E",
     "notes": {},
     "annotation": {},
 }

cobra/io/json.py

@@ -172,11 +172,6 @@ def load_json_model(filename):
                         "type": "number",
                         "default": 0,
                     },
-                    "variable_kind": {
-                        "type": "string",
-                        "pattern": "integer|continuous",
-                        "default": "continuous"
-                    },
                     "subsystem": {"type": "string"},
                     "notes": {"type": "object"},
                     "annotation": {"type": "object"},
@@ -203,11 +198,6 @@ def load_json_model(filename):
                         "type": "number",
                         "default": 0
                     },
-                    "_constraint_sense": {
-                        "type": "string",
-                        "default": "E",
-                        "pattern": "E|L|G",
-                    },
                     "notes": {"type": "object"},
                     "annotation": {"type": "object"},
                 },

cobra/io/mat.py

@@ -153,8 +153,6 @@ def create_mat_dict(model):
     mat["rxns"] = _cell(rxns.list_attr("id"))
     mat["rxnNames"] = _cell(rxns.list_attr("name"))
     mat["subSystems"] = _cell(rxns.list_attr("subsystem"))
-    mat["csense"] = "".join((
-        met._constraint_sense for met in model.metabolites))
     stoich_mat = create_stoichiometric_matrix(model)
     mat["S"] = stoich_mat if stoich_mat is not None else [[]]
     # multiply by 1 to convert to float, working around scipy bug

cobra/test/test_core/test_model.py

@@ -784,8 +784,9 @@ def test_remove_reactions(model):
 def test_objective(model):
     obj = model.objective
     assert obj.get_linear_coefficients(obj.variables) == {
-               model.variables["Biomass_Ecoli_core_reverse_2cdba"]: -1,
-               model.variables["Biomass_Ecoli_core"]: 1}
+        model.variables["Biomass_Ecoli_core_reverse_2cdba"]: -1,
+        model.variables["Biomass_Ecoli_core"]: 1
+    }
     assert obj.direction == "max"
 
 
@@ -886,17 +887,16 @@ def test_invalid_solver_change_raises(model):
 
 @pytest.mark.skipif('cplex' not in solvers, reason='no cplex')
 def test_change_solver_to_cplex_and_check_copy_works(model):
-    assert round(abs(model.optimize().f - 0.8739215069684306), 7) == 0
+    assert (model.slim_optimize() - 0.8739215069684306) == pytest.approx(0.0)
     model_copy = model.copy()
-    assert round(abs(model_copy.optimize().f - 0.8739215069684306),
-                 7) == 0
+    assert (model_copy.slim_optimize() - 0.8739215069684306) == pytest.approx(
+        0.0)
     # Second, change existing glpk based model to cplex
     model.solver = 'cplex'
-    assert round(abs(model.optimize().f - 0.8739215069684306),
-                 7) == 0
+    assert (model.slim_optimize() - 0.8739215069684306) == pytest.approx(0.0)
     model_copy = copy(model)
-    assert round(abs(model_copy.optimize().f - 0.8739215069684306),
-                 7) == 0
+    assert (model_copy.slim_optimize() - 0.8739215069684306) == pytest.approx(
+        0.0)
 
 
 def test_copy_preserves_existing_solution(solved_model):

cobra/test/test_flux_analysis/test_parsimonious.py

@@ -29,10 +29,11 @@ def test_pfba(model, all_solvers):
     n_constraints = len(model.constraints)
     solution = pfba(model)
     assert solution.status == "optimal"
-    assert np.isclose(solution.x_dict["Biomass_Ecoli_core"],
-                      0.8739, atol=1e-4, rtol=0.0)
-    abs_x = [abs(i) for i in solution.x]
-    assert np.isclose(sum(abs_x), 518.4221, atol=1e-4, rtol=0.0)
+    assert solution.fluxes["Biomass_Ecoli_core"] == \
+        pytest.approx(0.8739, abs=1e-4, rel=0.0)
+    assert solution.fluxes.abs().sum() == \
+        pytest.approx(518.4221, abs=1e-4, rel=0.0)
+
     # test changes to model reverted
     assert expression == model.objective.expression
     assert len(model.constraints) == n_constraints
@@ -51,10 +52,10 @@ def test_pfba(model, all_solvers):
     # Test fraction_of_optimum
     solution = pfba(model, fraction_of_optimum=0.95)
     assert solution.status == "optimal"
-    assert np.isclose(solution.x_dict["Biomass_Ecoli_core"],
-                      0.95 * 0.8739, atol=1e-4, rtol=0.0)
-    abs_x = [abs(i) for i in solution.x]
-    assert np.isclose(sum(abs_x), 493.4400, atol=1e-4, rtol=0.0)
+    assert solution.fluxes["Biomass_Ecoli_core"] == pytest.approx(
+        0.95 * 0.8739, abs=1e-4, rel=0.0)
+    abs_x = [abs(i) for i in solution.fluxes.values]
+    assert sum(abs_x) == pytest.approx(493.4400, abs=1e-4, rel=0.0)
 
     # Infeasible solution
     model.reactions.ATPM.lower_bound = 500

cobra/test/test_manipulation.py

@@ -20,24 +20,29 @@ def test_modify_reversible(self, model):
         model2 = model.copy()
         convert_to_irreversible(model2)
         solution2 = model2.optimize()
-        assert abs(solution1.f - solution2.f) < 10 ** -3
+        assert abs(
+            solution1.objective_value - solution2.objective_value) < 10 ** -3
         revert_to_reversible(model2)
         solution2_rev = model2.optimize()
-        assert abs(solution1.f - solution2_rev.f) < 10 ** -3
+        assert abs(
+            solution1.objective_value - solution2_rev.objective_value
+        ) < 10 ** -3
         # Ensure revert_to_reversible is robust to solutions generated both
         # before and after reversibility conversion, or not solved at all.
         model3 = model.copy()
         solution3 = model3.optimize()
         convert_to_irreversible(model3)
         revert_to_reversible(model3)
-        assert abs(solution1.f - solution3.f) < 10 ** -3
+        assert abs(
+            solution1.objective_value - solution3.objective_value) < 10 ** -3
         # test reaction where both bounds are negative
         model4 = model.copy()
         glc = model4.reactions.get_by_id("EX_glc__D_e")
         glc.upper_bound = -1
         convert_to_irreversible(model4)
         solution4 = model4.optimize()
-        assert abs(solution1.f - solution4.f) < 10 ** -3
+        assert abs(
+            solution1.objective_value - solution4.objective_value) < 10 ** -3
         glc_rev = model4.reactions.get_by_id(glc.notes["reflection"])
         assert glc_rev.lower_bound == 1
         assert glc.upper_bound == 0