From 63ce0577f85cedac8531b79865d73dd5977d1f12 Mon Sep 17 00:00:00 2001 From: "uri.akavia" Date: Thu, 11 Aug 2022 13:07:55 -0400 Subject: [PATCH] fixed according to comments --- src/cobra/core/gene.py | 26 +++++++++++++-- src/cobra/core/metabolite.py | 24 ++++++++++++-- src/cobra/core/model.py | 59 +++++++++++++-------------------- src/cobra/core/reaction.py | 64 +++++++++++++++++++++++++++++++----- src/cobra/core/species.py | 30 +++-------------- 5 files changed, 128 insertions(+), 75 deletions(-) diff --git a/src/cobra/core/gene.py b/src/cobra/core/gene.py index 0d1f86f29..5f4212dee 100644 --- a/src/cobra/core/gene.py +++ b/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""" diff --git a/src/cobra/core/metabolite.py b/src/cobra/core/metabolite.py index 6b3d31dc9..69be504c2 100644 --- a/src/cobra/core/metabolite.py +++ b/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, diff --git a/src/cobra/core/model.py b/src/cobra/core/model.py index ab0d40d8a..22851606f 100644 --- a/src/cobra/core/model.py +++ b/src/cobra/core/model.py @@ -723,34 +723,24 @@ def existing_filter(rxn: Reaction) -> bool: return True # First check whether the reactions exist in the model. Copy reactions that - # belong to another model, in order not to remove them (see - # https://github.com/opencobra/cobrapy/issues/673#issuecomment-371361476). - exist_but_different_stoich = set() + # 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): - met_id_dict = { - met.id: stoich for met, stoich in reaction.metabolites.items() - } - existing_met_id_dict = { - met.id: stoich - for met, stoich in self.reactions.get_by_id( - reaction.id - ).metabolites.items() - } - if met_id_dict != existing_met_id_dict: - exist_but_different_stoich.add(reaction.id) - other_model = reaction.model - if len(exist_but_different_stoich): + 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_stoich)} exist" + f"The reactions {', '.join(exist_but_different)} exist" f" in both {self} and {other_model}, but have different " - f"stoichiometries in the two mdoels. Please check to see " - f"which stoichiometry you'd like to add." + 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 + if not reaction.model or reaction.model == self else reaction.copy() for reaction in filter(existing_filter, reaction_list) ] @@ -763,20 +753,15 @@ def existing_filter(rxn: Reaction) -> bool: reaction._model = self if context: context(partial(setattr, reaction, "_model", None)) - met_id_dict = { - met.id: stoich for met, stoich in reaction.metabolites.items() - } - met_dict = {met.id: met for met in reaction.metabolites.keys()} mets_to_add = [ - met_dict[met_id] - for met_id in set(met_id_dict.keys()).difference( - self.metabolites.list_attr("id") - ) + 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_id, stoich in met_id_dict.items() + self.metabolites.get_by_id(met.id): stoich + for met, stoich in reaction.metabolites.items() } reaction.metabolites = new_stoich reaction.update_genes_from_gpr() @@ -846,15 +831,15 @@ def remove_reactions( self.reactions.remove(reaction) reaction._model = None - for met in reaction.metabolites: + for met in reaction._metabolites: if reaction in met._reaction: - met.reaction_remove(reaction, context) + met.remove_reaction(reaction) if remove_orphans and len(met.reactions) == 0: self.remove_metabolites(met) - for gene in reaction.genes: + for gene in reaction._genes: if reaction in gene._reaction: - gene.reaction_remove(reaction, context) + gene.remove_reaction(reaction) if remove_orphans and len(gene.reactions) == 0: self.genes.remove(gene) @@ -1287,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) + met.add_reaction(rxn) # point _model to self for dict_list in (self.reactions, self.genes, self.metabolites, self.groups): diff --git a/src/cobra/core/reaction.py b/src/cobra/core/reaction.py index dc2c9a2c4..1b463bae7 100644 --- a/src/cobra/core/reaction.py +++ b/src/cobra/core/reaction.py @@ -1,6 +1,7 @@ """Define the Reaction class.""" import hashlib +import math import re from collections import defaultdict from copy import copy, deepcopy @@ -594,7 +595,7 @@ def metabolites(self, value: Dict[Metabolite, float]) -> None: self._metabolites = value context = get_context(self) for met in value.keys(): - met.reaction_add(self, context) + met.add_reaction(self) @property def genes(self) -> FrozenSet: @@ -858,9 +859,9 @@ def _update_awareness(self) -> None: This function is deprecated and shouldn't be used. """ for x in self._metabolites: - x.reaction_add(self) + x.add_reaction(self) for x in self._genes: - x.reaction_add(self) + x.add_reaction(self) def remove_from_model(self, remove_orphans: bool = False) -> None: """Remove the reaction from a model. @@ -951,7 +952,7 @@ def __setstate__(self, state: Dict) -> None: self.__dict__.update(state) for x in set(state["_metabolites"]).union(state["_genes"]): x._model = self._model - x.reaction_add(self) + x.add_reaction(self) def copy(self) -> "Reaction": """Copy a reaction. @@ -1001,7 +1002,7 @@ def __add__(self, other: "Reaction") -> "Reaction": """ new_reaction = self.copy() - if other == 0: + if isinstance(other, int) and other == 0: return new_reaction else: new_reaction += other @@ -1285,7 +1286,7 @@ def add_metabolites( self._metabolites[metabolite] = coefficient # make the metabolite aware that it is involved in this # reaction - metabolite.reaction_add(self) + metabolite.add_reaction(self) # from cameo ... model = self.model @@ -1304,7 +1305,7 @@ def add_metabolites( if the_coefficient == 0: # make the metabolite aware that it no longer participates # in this reaction - metabolite.reaction_remove(self) + metabolite.remove_reaction(self) self._metabolites.pop(metabolite) context = get_context(self) @@ -1504,7 +1505,7 @@ def _associate_gene(self, cobra_gene: Gene) -> None: """ self._genes.add(cobra_gene) - cobra_gene.reaction_add(self) + cobra_gene.add_reaction(self) cobra_gene._model = self._model def _dissociate_gene(self, cobra_gene: Gene) -> None: @@ -1516,7 +1517,7 @@ def _dissociate_gene(self, cobra_gene: Gene) -> None: """ self._genes.discard(cobra_gene) - cobra_gene.reaction_remove(self) + cobra_gene.remove_reaction(self) def knock_out(self) -> None: """Knockout reaction by setting its bounds to zero.""" @@ -1673,6 +1674,51 @@ def summary( fva=fva, ) + def __eq__(self, other) -> bool: + self_state = self.__getstate__() + self_state["_metabolites"] = { + met.id: stoic for met, stoic in self.metabolites.items() + } + self_state["_genes"] = {gene.id for gene in self.genes} + self_state.pop("_model") + other_state = other.__getstate__() + other_state["_metabolites"] = { + met.id: stoic for met, stoic in other.metabolites.items() + } + other_state["_genes"] = {gene.id for gene in other.genes} + other_state.pop("_model") + + self_state.pop("_lower_bound") + self_state.pop("_upper_bound") + other_state.pop("_lower_bound") + other_state.pop("_upper_bound") + + _tolerance = 1e-7 + if self.model: + _tolerance = self.model.tolerance + elif other.model: + _tolerance = other.model.tolerance + elif self.model and other.model: + _tolerance = min(self.model.tolerance, other.model.tolerance) + if not math.isclose(self.lower_bound, other.lower_bound, abs_tol=_tolerance): + return False + elif not math.isclose(self.upper_bound, other.upper_bound, abs_tol=_tolerance): + return False + else: + return self_state == other_state + + def __hash__(self) -> int: + """Define __hash__ explicitly to allow usage of reaction in sets. + + Python objects that define __eq__ MUST define __hash__ explicitly, or they + are unhashable. __hash__ is set to the __hash__ function of cobra.Object. + + Returns + ------- + int + """ + return Object.__hash__(self) + def __str__(self) -> str: """Return reaction id and reaction as str. diff --git a/src/cobra/core/species.py b/src/cobra/core/species.py index 658b5d55a..162e0361c 100644 --- a/src/cobra/core/species.py +++ b/src/cobra/core/species.py @@ -2,11 +2,9 @@ from copy import deepcopy -from functools import partial from typing import TYPE_CHECKING, FrozenSet, Optional from ..core.object import Object -from ..util import HistoryManager if TYPE_CHECKING: @@ -57,49 +55,31 @@ def reactions(self) -> FrozenSet: FrozenSet A frozenset that includes the reactions of the species. """ - if self.model and self.__class__.__name__ == "Gene": - return frozenset(self.model.reactions.query(lambda x: self in x, "genes")) - elif self.model and self.__class__.__name__ == "Metabolite": - return frozenset( - self.model.reactions.query(lambda x: self in x, "metabolites") - ) return frozenset(self._reaction) - def reaction_add( - self, reaction: "Reaction", context: Optional[HistoryManager] = None - ) -> None: + def add_reaction(self, reaction: "Reaction") -> None: """Add reaction to .reaction field, with context. - If this is called with a context, will be reversed when exiting the context. + If this is called within a context, will be reversed when exiting the context. Parmeters --------- reaction: cobra.Reaction - context: HistoryManager, optional - context this action is in, optional (defualt None). """ self._reaction.add(reaction) - if context: - context(partial(self._reaction.remove, reaction)) - def reaction_remove( - self, reaction: "Reaction", context: Optional[HistoryManager] = None - ) -> None: + def remove_reaction(self, reaction: "Reaction") -> None: """Remove reaction from .reaction field, with context. - If this is called with a context, will be reversed when exiting the context. + If this is called within a context, will be reversed when exiting the context. Parmeters --------- reaction: cobra.Reaction - context: HistoryManager, optional - context this action is in, optional (defualt None). """ self._reaction.remove(reaction) - if context: - context(partial(self._reaction.add, reaction)) - def reaction_clear(self) -> None: + def clear_reaction(self) -> None: """Clear the reaction field.""" self._reaction.clear()