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

ftINIT helpers #388

Merged
merged 8 commits into from
Jun 14, 2022
Merged

ftINIT helpers #388

merged 8 commits into from
Jun 14, 2022

Conversation

johan-gson
Copy link
Collaborator

Main improvements in this PR:

*Added help functions that are used together with ftINIT in RAVEN. A pull request containing the ftINIT implementation has been done separately in RAVEN, but the changes should not be dependent on each other.

I hereby confirm that I have:

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

johan-gson added 4 commits May 5, 2022 08:10
…man-GEM to RAVEN - these are used by ftINIT.
…ious tINIT since the RAVEN function has been tagged with '_legacy'.
…ed legacy. This makes the pull requests in RAVEN and Human-GEM independent. Let's fix this at some later point.
% Usage: rxns=getAATripletReactions(model, onlyRxnsWithoutGPRs)
%

aminoAcidsCap = {'Alanine';'Arginine';'Asparagine';'Aspartate';'Cysteine';'Glutamine';'Glutamate';'Glycine';'Histidine';'Isoleucine';'Leucine';'Lysine';'Methionine';'Metheonine';'Phenylalanine';'Proline';'Serine';'Threonine';'Tryptophan';'Tyrosine';'Valine'};
Copy link
Collaborator

@JonathanRob JonathanRob May 20, 2022

Choose a reason for hiding this comment

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

Not sure what Metheonine is, but it can probably be removed (should only be 20 AAs)

Edit: I stand corrected, apparently it exists in the model. My bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, the problem is that there are reactions where the amino acids are misspelled - that's why I have added those weird ones :)

Copy link
Member

Choose a reason for hiding this comment

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

How about submitting another PR that fixes the misspelling, and merging that one first?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah sorry, I should have actually looked into it further before commenting.

Copy link
Member

@mihai-sysbio mihai-sysbio May 20, 2022

Choose a reason for hiding this comment

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

Is there a reason to not code in all 22 amino acids?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, the point of this function is to identify reactions that convert amino acid triplets back and forth, i.e. chains of three amino acids <=> three separate amino acids. It is a measure of optimization - these reactions are pretty pointless in most practical uses with tINIT. It is like 700 reactions or something like that. So I only added the amino acids that help identify those reactions.

@mihai-sysbio mihai-sysbio changed the title Ft init helpers ftINIT helpers May 20, 2022
@@ -1,5 +1,7 @@
function [model, metProduction, essentialRxnsForTasks, addedRxnsForTasks, deletedDeadEndRxns, deletedRxnsInINIT, taskReport] = getINITModel2(refModel, tissue, celltype, hpaData, arrayData, metabolomicsData, removeGenes, taskFile, useScoresForTasks, printReport, taskStructure, params, paramsFT)
% getINITModel2
% This function is deprecated - we suggest that you instead use ftINIT in
% RAVEN Toolbox together with the helpers in this repo.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have mixed feelings about this. This function is still (nearly) the original implementation of the algorithm, and is not necessarily "deprecated". Although ftINIT is faster, it uses a different approach (with additional assumptions and simplifications). So unless we can definitively show that ftINIT is a more accurate algorithm, I hesitate to "deprecate" the original tINIT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The question is what to write here. Should we say that there is a newer version, but not call it deprecated? I have some reason to believe that ftINIT is performing better. I'm supplying a powerpoint that shows one test I did with this function and compared it to tINIT without allowing flux in both direction and without metabolite secretion. ftINIT in this test gave the same result as the one where these were not allowed, while the standard getINITModel2 produced something that is quite different, full of gaps. The task step of ftINIT typically fills in ~13 rxns, while the same step for getINITModel2 fills in ~55 rxns. So, I think that getINITModel2 has some problems, and that the problems with ftINIT are much smaller. But I have not tested it super much.
tINITTestInfo.pptx

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, very good points. I think it's fine to state that there is a newer version, but would avoid using the word "deprecated", since it sometimes implies that the function will eventually be removed (which is not the intent).

I agree that some of the behavior you've seen with ftINIT (other than shorter compute time) can be interpreted as a clear improvement in performance/accuracy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I wrote that there is a new improved version of the function available in RAVEN, I have pushed a new commit regarding this.

'MAR10063'; ...
'MAR10064'; ...
'MAR10065'; ...
'MAR13082'};
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a lot of hard-coded items in this function, which I generally would like to avoid. But since I don't really have better suggestions, I think it's fine to leave like this.

'MAR09818'};

%reactions that just pool metabolites
poolRxns = { ...
Copy link
Member

Choose a reason for hiding this comment

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

Pool reactions are reactions that have pool in the name, so these could be defined dynamically.

function outModel=removeDrugReactions(inModel)
% outModel=removeDrugReactions(inModel)
%
% Removes reactions from Human-GEM related to drugs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would be easier to just remove all reactions that are in the Drug metabolism subsystem. I imagine it might remove a few additional reactions, but unlikely that they would be important for anything. Something to think about in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha, I never thought about that.

Copy link
Member

Choose a reason for hiding this comment

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

As a review, I have following concerns to this function:

  • there is some commented lousy code
  • it doesn't appear to be generic enough for future use, thus might be okay to be hard coded inside prepHumanModelForftINIT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure that should be there - this is to show how I tested that the code works. Maybe we could clean that up.

Copy link
Member

Choose a reason for hiding this comment

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

yes please

@JonathanRob
Copy link
Collaborator

Ok, I'm still going through some of the details on this PR, though so far it looks ok to me. But why are the yaml conversion tests failing?

@johan-gson
Copy link
Collaborator Author

Yeah, I don't get that either, I cannot see that I have done anything with that. What I understood from the error messages is that it fails to allocate some resource for running them?

@mihai-sysbio
Copy link
Member

mihai-sysbio commented May 24, 2022

Ok, I'm still going through some of the details on this PR, though so far it looks ok to me. But why are the yaml conversion tests failing?

Now resolved, the Actions runner was offline.

@haowang-bioinfo haowang-bioinfo merged commit cf02955 into develop Jun 14, 2022
@mihai-sysbio mihai-sysbio deleted the ftINITHelpers branch June 14, 2022 04:19
@mihai-sysbio mihai-sysbio mentioned this pull request Jun 17, 2022
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