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

feat: change multiple compartment abbreviations #341

Merged
merged 10 commits into from
Jan 24, 2022

Conversation

JonathanRob
Copy link
Collaborator

@JonathanRob JonathanRob commented Dec 19, 2021

Main improvements in this PR:

This is a more extensive/exhaustive implementation of #330 to address discussions in #314.

Three Human-GEM compartment abbreviations are changed in the model file, metabolite annotation file, metabolic task files, and the addBoundaryMets function:

Compartment Abbreviation New Abbreviation
Extracellular s e
Boundary x b
Peroxisome p x

Since the changes (diffs) are quite large, I implemented the changes with a script changeCompAbbrevs in code/modelCuration/.

I hereby confirm that I have:

  • Tested my code on my own computer for running the model
  • Selected develop as a target branch

Copy link

@smoretti smoretti left a comment

Choose a reason for hiding this comment

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

I did not know all your code and files but the ones I know look OK

Copy link
Member

@mihai-sysbio mihai-sysbio left a comment

Choose a reason for hiding this comment

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

That's neat!
Just so you know, there is a reference to 'x' in a commented line in importYaml, which might be good to either update or delete.

@JonathanRob
Copy link
Collaborator Author

Just so you know, there is a reference to 'x' in a commented line in importYaml, which might be good to either update or delete.

@mihai-sysbio Good catch, I have now removed that line.

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Dec 26, 2021

I've gone through all the changes that look good to me. @JonathanRob nice work.

though there's a little concern about commit 5e6af0f that marked all these previous metabolite ids as deprecated, this is probably overkill. What we are trying to do here is replacing ids i.e. adapting to the BiGG (or KBase/SEED) scheme (#314), which actually reserved p and s for compartments periplasm and eyespot, respectively. And this commit pre-excluded the possibility of their occurrence in periplasm and/or eyespot (even though HumanGEM doesn't include these compartments at the moment, they might be added later). So this commit appears to be unnecessary and should be abandoned. What do you guys think?

@haowang-bioinfo
Copy link
Member

now the changes introduced by #339 led to conflicts in metabolites.tsv

@mihai-sysbio
Copy link
Member

It looks to me like we are following the BiGG specification here, but maybe we should reference the source directly.
For ModelSEED the template for Human on the master branch looks fairly compatible with implemented changes.

id name
c0 Cytosol
e0 Extracellular
a0 Carboxysome
g0 Golgi
l0 Lysosome
m0 Mitochondria
n0 Nucleus
r0 Endoplasmic_Reticulum
x0 Peroxisome

@mihai-sysbio
Copy link
Member

Just a ping to advance this PR.
@JonathanRob do you think you can resolve the conflicts?
@haowang-bioinfo is it okay that we are waiting for your review?

@JonathanRob
Copy link
Collaborator Author

Once #343 is merged, the conflicts should be resolved.

@mihai-sysbio
Copy link
Member

To avoid future merge conflicts, I think it makes sense to hold off on creating any PRs until the metabolites get renamed. This makes the current PR a priority. @JonathanRob let me know how I can help.

@JonathanRob
Copy link
Collaborator Author

@mihai-sysbio yes, exactly. I will merge #343 to see if that solves the conflict on this PR. If that doesn't work, then I may ask for some help.

@JonathanRob JonathanRob merged commit 47a1e31 into develop Jan 24, 2022
@JonathanRob JonathanRob deleted the feat/change_comp_abbrevs branch January 24, 2022 08:47
@haowang-bioinfo
Copy link
Member

@haowang-bioinfo is it okay that we are waiting for your review?

@mihai-sysbio sorry I missed this message

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

Successfully merging this pull request may close these issues.

4 participants