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

[WIP] Multiplex reporting #2286

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Alexander-Sol
Copy link
Contributor

Small refactor to multiplex ion quantification to increase speed and code quality. Introduced MultiplexMod class to handle all multiplex operations.

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #2286 (ebbddb4) into master (2729265) will decrease coverage by 0.01%.
The diff coverage is 99.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2286      +/-   ##
==========================================
- Coverage   91.92%   91.92%   -0.01%     
==========================================
  Files         133      134       +1     
  Lines       20272    20290      +18     
  Branches     2824     2826       +2     
==========================================
+ Hits        18635    18651      +16     
- Misses       1156     1157       +1     
- Partials      481      482       +1     
Impacted Files Coverage Δ
MetaMorpheus/EngineLayer/Mods/MultiplexMod.cs 98.64% <98.64%> (ø)
MetaMorpheus/EngineLayer/PeptideSpectralMatch.cs 98.84% <100.00%> (+0.01%) ⬆️
...eus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs 92.73% <100.00%> (-0.32%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Member

@nbollis nbollis left a comment

Choose a reason for hiding this comment

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

Overall looks great, just had two questions before aproval

return ionLabels;
}

public double[] GetMultiplexIonIntensities(MzSpectrum scan, double ppmTolerance = 10)
Copy link
Member

Choose a reason for hiding this comment

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

Is this tolerance settable by the user? If not, should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, but honestly I haven't thought about it too deeply. Ten is a heuristic value that seems to be working, but it's possible that we could implement it as a user specified value in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

tolerance should match fragment ion tolerance for the file


foreach(var psm in Parameters.AllPsms)
{
psm.SetMultiplexIonIntensities(MultiplexMod);
}
return;
Copy link
Member

Choose a reason for hiding this comment

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

For each psm, are you only setting the multiplex (diagnostic) ion intensities of a single modification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yessir. When Multiplex Quant is enabled, the user can only select one type of multiplex modification (e.g., TMT11 on K + TMT11 on N-terminus). We only need to store the ion intensities for that one multiplex mod.

@Alexander-Sol Alexander-Sol added WIP Work in progress, not ready for review and removed ready for review labels Jun 21, 2023
Copy link

@kyp4 kyp4 left a comment

Choose a reason for hiding this comment

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

Looks good, no problems running

@Alexander-Sol Alexander-Sol changed the title Multiplex reporting [WIP] Multiplex reporting Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress, not ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants