initial attempt at compartments #1193
Conversation
|
I think for efficiency it would be nice if it could derive from the Group class already implemented in core/group.py. You could add new attributes specific to the compartment. |
src/cobra/core/compartment.py
Outdated
|
|
||
|
|
||
| class Compartment(Object): | ||
| class Compartment(Group): | ||
| """ | ||
| Manage groups via this implementation of the SBML group specification. |
There was a problem hiding this comment.
This is probably wrong. First sentence should describe Compartment not Group. Better Documentation of class needed.
src/cobra/core/compartment.py
Outdated
| dimensions: Optional[float] = None, | ||
| ): | ||
| def __init__(self, id: str, name: str = "", members: Optional[Iterable] = None, | ||
| dimensions: Optional[float] = None): | ||
| """Initialize the group object. |
There was a problem hiding this comment.
"Initialize the Compartment object."
|
|
||
| Raises | ||
| ------ | ||
| TypeError if given anything other than metaoblites or reactions. |
| @@ -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 | |||
| @@ -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 | |||
| """ | |||
| if isinstance(new_members, str) or hasattr(new_members, "id"): | |||
There was a problem hiding this comment.
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`.
| @@ -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]: | |||
There was a problem hiding this comment.
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
| 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) |
There was a problem hiding this comment.
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!
|
|
||
|
|
||
| class Compartment(Object): | ||
| class Compartment(Group): |
There was a problem hiding this comment.
I don't know if inheriting from Group is really desirable or we should just have a similar class interface.
There was a problem hiding this comment.
Another point in favor is that .members is a bit awkward here. We rather want attributes .reactions and .metabolites.
| def __contains__(self, member: Union[Metabolite, Gene]): | ||
| return member.compartment is self | ||
| def __contains__(self, member: Union[Metabolite, Reaction]): | ||
| return self.members.__contains__(member) |
There was a problem hiding this comment.
If we keep members then more pythonic would be
| return self.members.__contains__(member) | |
| return member in self.members |
Very initial attempt at compartments. Just to see if I'm heading in the right direction. Will not be committed as is.