opencobra / cobrapy

COBRApy is a package for constraint-based modeling of metabolic networks.
http://opencobra.github.io/cobrapy/
GNU General Public License v2.0
466 stars 218 forks source link

Reactions are unaware of groups (and subsystems) #938

Open akaviaLab opened 4 years ago

akaviaLab commented 4 years ago

Problem description

The model has groups that contain members (list of reactions). However, the reactions do not have a groups field, and the subsystems is always an empty string. I think it should behave like genes, where reaction has a set of groups and relevant functions _associate_group(self, cobra_group) _dissociate_group(self, cobra_group)

If that's approved, models that include groups should associate them with reactions when loaded.

This is similar, but different from #543 and #473 - I understand this should be encoded as groups, but I think reactions should know their groups.

I would suggest having subsystems either be 1) Deprecated 2) Just a link to groups, basically reaction.subsystem = reaction.groups 3) A list of textual group IDs and not full group members (similar idea to GPR string vs genes)

If the key developers can comment on this.

gregmedlock commented 4 years ago

I think the main implementation concern is being careful with circular references as much as possible so that deletion and addition of objects doesn't mangle group membership. This is probably why _associate_gene and _dissociate_gene are written as "private" methods.

I think this would be a welcome feature but would require a lot of work on the API (and might seem weird if functions for associating genes with reactions are still private). Implementing it in Object might be a good option so that any of the cobrapy objects can be associated/dissociated with a group via this method.

akaviaLab commented 4 years ago

I'm a bit confused. I wasn't planning to use _associate_gene, just make an equivalent for _associate_group, which I guess will be private as well. Are entities other than reaction associated with groups?

cdiener commented 4 years ago

It's true that Reaction.subsystem is a relic of SBML 2 times. We had a lot of bugs because of multiple references as @gregmedlock mentioned when we introduced the context managers. This is why I pushed for a "single source of truth" model where only one entity tracks connections between objects. A metabolite does not know what reactions it belongs to. Metabolite.reaction actually goes through all reactions and returns the ones that the metabolite belongs to. That is a bit slower but way more stable in terms of maintenance.

It would be pretty easy though to do the same thing for Reaction.groups and I think that would be a good contribution.

matthiaskoenig commented 4 years ago

Quick input:

Groups are much more powerful then the subsystems, and the information in subsystems is only a subset of the possible group information.