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

Reactions are unaware of groups (and subsystems) #938

Open
akaviaLab opened this issue Feb 26, 2020 · 4 comments
Open

Reactions are unaware of groups (and subsystems) #938

akaviaLab opened this issue Feb 26, 2020 · 4 comments
Assignees

Comments

@akaviaLab
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

cdiener commented Feb 27, 2020

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
Copy link
Contributor

Quick input:

  • yes, other objects then reactions are in groups, in the context of cobrapy the relevant objects are reaction, species, gene, compartment (and could also be a GPR).
  • single objects can be part of many groups, i.e. a species like atp could be in many groups.
  • groups can be hierarchical, i.e. groups could contain other groups

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

@gregmedlock gregmedlock self-assigned this Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants