Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial attempt at compartments #1193

Draft
wants to merge 4 commits into
base: devel
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 66 additions & 84 deletions src/cobra/core/compartment.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
from warnings import warn

from .dictlist import DictList
from .object import Object
from .. import Metabolite, Gene, Reaction, Model
from .group import Group
from .. import Metabolite, Reaction, Model


class Compartment(Object):
class Compartment(Group):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if inheriting from Group is really desirable or we should just have a similar class interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another point in favor is that .members is a bit awkward here. We rather want attributes .reactions and .metabolites.

"""
Manage groups via this implementation of the SBML group specification.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably wrong. First sentence should describe Compartment not Group. Better Documentation of class needed.


Expand All @@ -26,35 +26,15 @@ class Compartment(Object):
A DictList containing references to cobra.Model-associated objects
that belong to the group. Members should be metabolites or genes, where
reactions are inferred from metabolites.
compartment_type : str, optional
SBML Level 2 Versions 2–4 provide the compartment type as a grouping construct
that can be used to establish a relationship between multiple Compartment
objects. A CompartmentType object only has an identity, and this identity can
only be used to indicate that particular Compartment objects in the model
belong to this type. This may be useful for conveying a modeling intention,
such as when a model contains many similar compartments, either by their
biological function or the reactions they carry. Without a compartment type
construct, it would be impossible within SBML itself to indicate that all of
the compartments share an underlying conceptual relationship because each
SBML compartment must be given a unique and separate identity. A
CompartmentType has no mathematical meaning in SBML—it has no effect on
a model's mathematical interpretation. Simulators and other numerical analysis
software may ignore CompartmentType definitions and references in a model.
dimensions: float, optional
Compartments can have dimensions defined, if they are volume (3 dimensions) or
2 (a two-dimensional compartment, a surface, like a membrane). In theory, this
can be 1 or 0 dimensions, and even partial dimensions, but that will needlessly
complicate the math. The number of dimensions influences the size and units
used.
"""
def __init__(
self,
id: str,
name: str = "",
members: Optional[Iterable] = None,
compartment_type: Optional[str] = None,
dimensions: Optional[float] = None,
):
def __init__(self, id: str, name: str = "", members: Optional[Iterable] = None,
dimensions: Optional[float] = None):
"""Initialize the group object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Initialize the Compartment object."


id : str
Expand All @@ -64,69 +44,32 @@ def __init__(
members : iterable, optional
A DictList containing references to cobra.Model-associated objects
that belong to the group.
kind : {"collection", "classification", "partonomy"}, optional
The kind of group, as specified for the Groups feature in the SBML
level 3 package specification.
"""
Object.__init__(self, id, name)
dimensions: float, optional
akaviaLab marked this conversation as resolved.
Show resolved Hide resolved
Compartments can have dimensions defined, if they are volume (3 dimensions) or
2 (a two-dimensional compartment, a surface, like a membrane). In theory, this
can be 1 or 0 dimensions, and even partial dimensions. The number of
dimensions influences the size and units used.

Raises
------
TypeError if given anything other than metaoblites or reactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: metabolites

"""
super().__init__(id, name, members)
for x in members:
if not isinstance(x, (Metabolite, Reaction)):
raise(TypeError, f"Compartments should have only "
akaviaLab marked this conversation as resolved.
Show resolved Hide resolved
f"reactions or metabolites. {x} is a {type(x)}.")
self._members = DictList() if members is None else DictList(members)
self._compartment_type = None
self.compartment_type = "" if compartment_type is None else compartment_type
self.__delattr__("kind") # Compartments don't have kind
# self.model is None or refers to the cobra.Model that
# contains self
self._dimensions = dimensions
self._model = None

def __len__(self) -> int:
"""Get length of group.

Returns
-------
int
An int with the length of the group.

"""
return len(self._members)

# read-only
@property
def members(self) -> DictList:
"""Get members of the group.

Returns
-------
DictList
A dictlist containing the members of the group.
"""
return self._members

@property
def compartment_type(self) -> str:
"""Return the compartment type.

Returns
-------
str
The compartment type.

"""
return self._compartment_type

@compartment_type.setter
def compartment_type(self, compartment_type: str) -> None:
"""Set the compartment type.

Parameters
----------
compartment_type: str

"""
self._compartment_type = compartment_type

def add_members(self, new_members: Union[str, Metabolite, Reaction,
List[Union[Reaction, Metabolite]]]) -> None:
"""Add objects to the group.
"""Add objects to the compartment.

Parameters
----------
Expand All @@ -137,7 +80,7 @@ def add_members(self, new_members: Union[str, Metabolite, Reaction,

Raises
------
TypeError - if given any object other than Metaoblite or Gene
TypeError - if given any object other than Metaoblite or Reaction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "Metabolite"

"""
if isinstance(new_members, str) or hasattr(new_members, "id"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplication here. Same code below again. This is not very robust and the warning message is not informative. Be more precise, e.g. "members should be provided as list`.

warn("need to pass in a list")
Expand Down Expand Up @@ -180,25 +123,64 @@ def metabolites(self) -> DictList[Metabolite]:
return self._members.query(lambda x: isinstance(x, Metabolite))

@property
def reactions(self) -> Optional[FrozenSet[Reaction]]:
def inferred_reactions(self) -> FrozenSet[Reaction]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I always prefer to use names, so that code completion shows me the options below each other. I.e. reactions_inferred and reactions_assigned are already much better. Also be more precise. There could be many ways for inferral, so I would just state the method.
I.e. better names could be: reactions_from_metabolites and reactions_from_manual

"""Return the reactions whose metabolites are in the compartment.

This is returned as a FrozenSet of reactions for each metabolite in the
compartment, if any.

Returns
-------
FrozenSet of cobra.Reactions
Reactions that have metabolites that belong to this compartment.

Returns
-------
FrozenSet of cobra.Reactions
Reactions that have metabolites that belong to this compartment.
"""
direct_set = set(self._members.query(lambda x: isinstance(x, Reaction)))
rxn_set = set()
for met in self.metabolites:
rxn_set.update(met._reactions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems just wrong. I understand what you are doing is: "Adding all reactions with at least 1 metabolite in the compartment". What you should be doing is "Add only reactions with all metabolites in the compartment!". Please check this!

return frozenset(rxn_set.union(direct_set))
return frozenset(rxn_set)

@property
def assigned_reactions(self) -> FrozenSet[Reaction]:
"""Return the reactions who were assigned to this compartment.

This is returned as a FrozenSet of reactions for each metabolite in the
compartment, if any.

Returns
-------
FrozenSet of cobra.Reactions
Reactions that have metabolites that belong to this compartment.

Returns
-------
FrozenSet of cobra.Reactions
Reactions that were assigned to this compartment, if any.
"""
return frozenset(self._members.query(lambda x: isinstance(x, Reaction)))

@property
def reactions(self) -> FrozenSet[Reaction]:
akaviaLab marked this conversation as resolved.
Show resolved Hide resolved
"""Return the reactions who belong to this compartment.

This is returned as a FrozenSet of reactions for each metabolite in the
compartment, if any, and the reactions that were assigned to this compartment
directly.

Returns
-------
FrozenSet of cobra.Reactions
Reactions that belong to this compartment, both assigned and inferred.
"""
assigned_reactions = set(self.assigned_reactions)
return frozenset(assigned_reactions.union(self.inferred_reactions))

def __contains__(self, member: Union[Metabolite, Gene]):
return member.compartment is self
def __contains__(self, member: Union[Metabolite, Reaction]):
return self.members.__contains__(member)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep members then more pythonic would be

Suggested change
return self.members.__contains__(member)
return member in self.members


def merge(self, other):
warn("Not implemented yet")
Expand Down
13 changes: 13 additions & 0 deletions src/cobra/core/reaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
)
from warnings import warn

from .compartment import Compartment

if TYPE_CHECKING:
from optlang.interface import Variable
Expand Down Expand Up @@ -1439,6 +1440,18 @@ def get_compartments(self) -> list:
warn("use Reaction.compartments instead", DeprecationWarning)
return list(self.compartments)

@property
def location(self) -> Optional[Compartment]:
"""Get assigned compartment, if any.

Returns
-------
Compartment
Gets the compartment this reaction was explicitly assigned to, if one
exists. If no compartment was assigned, return None.
"""
return self._compartment

def _associate_gene(self, cobra_gene: Gene) -> None:
"""Associates a cobra.Gene object with a cobra.Reaction.

Expand Down