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

Feature/implement groups #754

Closed

Conversation

gregmedlock
Copy link
Member

Implements the group class and the Model.groups attribute for better compliance with the SBML FBC package, fixes #543.

Some key implementation choices:

  • reactions, metabolites, or genes can belong to groups
  • groups can only take "kind" values as specified in the FBC specification
  • each Group has a members attribute, which is a set of reactions, metabolites, and/or genes.
  • whenever reactions, metabolites, or genes are removed from a model, group membership is updated.
  • a Group that is not associated with a model can be generated in isolation and associated with cobrapy objects; upon addition of the Group to the model, all cobrapy objects associated with the Group are also added.
  • when a Group is removed from a model, no modifications are made to cobrapy objects that were previously members of the group.
  • model.get_associated_groups() is used to dynamically look up group membership. This PR does not have a model-based helper function for retrieving group information en masse (e.g. group membership info for all reactions, metabolites and genes with a single function)

Models needed to be repickled for tests to pass; one interesting artifact is a "version" tag in mini.json that is now being regenerated with a string in the corresponding field as "1" instead of the int (1) that is compliant with the jsonschema. I'm not sure what the source of this is, since it seems like it's been a while since the models were repickled in a PR. So, here I just manually corrected the issue in mini.json (changing "1" to 1).

I think I will add documentation for groups in a separate PR; would it make the most sense to add a section in "building a model" and "reading and writing a model"?

Code review and comments for improvement are much appreciated; I tried to keep things consistent with the rest of the codebase where possible. @matthiaskoenig please let me know if I missed some major functionality that would make the libsbml integration smoother!

@codecov-io
Copy link

codecov-io commented Aug 24, 2018

Codecov Report

Merging #754 into devel will decrease coverage by 0.33%.
The diff coverage is 68.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel     #754      +/-   ##
==========================================
- Coverage   82.87%   82.54%   -0.34%     
==========================================
  Files          40       41       +1     
  Lines        3883     3981      +98     
  Branches      885      911      +26     
==========================================
+ Hits         3218     3286      +68     
- Misses        463      489      +26     
- Partials      202      206       +4
Impacted Files Coverage Δ
cobra/core/gene.py 73.91% <100%> (+0.69%) ⬆️
cobra/manipulation/delete.py 82.4% <33.33%> (-1.41%) ⬇️
cobra/core/model.py 86.12% <58.33%> (-4.08%) ⬇️
cobra/core/group.py 87.87% <87.87%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ee8eaa...220c1ed. Read the comment docs.

@Midnighter
Copy link
Member

I haven't had the time yet to review your code but your description of the PR sounds really promising!

The problem with the JSON version that you see is related to #720 and will be fixed in #725 where we switch to only strings. So no manual editing to stick with the current schema is necessary.

@gregmedlock
Copy link
Member Author

gregmedlock commented Aug 27, 2018

Ok, I reran update_pickles.py so the files represent their true state. Also did it to retrigger the travis build; looks like the python 2.7 build timed out without any error.

edit: leaving mini.json as is makes the travis build fail, but tests were fine before.

Copy link
Member

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

I made a few changes to the class interface, for example, no assignment to .members only via .add_members. The only problem right now is that cobra.Model has no add_genes methods. We need to fix that first. Also, the other tests will pass once the schema is updated via the other open PR.

@Midnighter Midnighter force-pushed the feature/implement_groups branch from b0cc984 to 2558642 Compare September 6, 2018 11:17
@Midnighter Midnighter added the WIP work in progress label Oct 18, 2018
@Midnighter
Copy link
Member

Hi @gregmedlock, thanks again for your work here.Your code has been merged as part of #685. I have some code locally that will extend the JSON schema with groups. So that still needs to be done but this code is finally in devel. I'm closing the PR because your code was merged locally.

@Midnighter Midnighter closed this Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add groups feature
3 participants