-
Notifications
You must be signed in to change notification settings - Fork 33
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
Spectral decon #675
base: master
Are you sure you want to change the base?
Spectral decon #675
Conversation
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
==========================================
- Coverage 81.12% 80.98% -0.14%
==========================================
Files 147 148 +1
Lines 25690 25762 +72
==========================================
+ Hits 20840 20863 +23
- Misses 4850 4899 +49
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Looks like there are a few places to add tests for default value returns and exception throws.
- Was there a dependency problem that led to needing to change where
Proteomics
is located?
The new deconvolution method relies on generating a library of theoretical isotopic distributions for all species in a given protein database. To do so, I need access to classes inside of Proteomics (e.g. PeptideWithSetModifications) |
Within Proteomics folder, it appears as through some things have specific namespaces while others do not: Would it make more sense to keep the namespaces of each file the same as they were before the refactoring? This would enable continuous integration as we would not need to change any namespace references in programs that implement MzLib, but would still allow for them to be in the same project, as was required for your PR. |
Historically, this comes from borrowing code from https://github.com/dbaileychess/CSMSL, which has a bit more specific namespace usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alex and I conducted an in-person review over the basics of the algorithm and checked the outputs. The outputs make sense, the deconvolution added is very extendable. It doesn't affect how MetaMorpheus currently deconvolutes thanks to @nbollis's refactoring of the deconvolution code to support a well-executed design pattern.
I think it's a good time to merge this new set of code into master so we can continue working on deconvolution while maintaining a linear-ish commit history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. You could consider going through your comments one more time and removing ones that aren't meant for perpetuity. There are also quite a few TODOs to review; should these be completed before merging this? Or are they more for future note (could use something other than TODO)?
There's also a pretty big refactor of the Proteomics
namespace. Were there any changes in there that are hard to review because of moving those files?
} | ||
else | ||
{ | ||
//TODO: Add some averagine bullshit here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha this one's almost worth framing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need to add something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not on this PR. That would come in subsequent iterations
public List<Modification> VariableModifications { get; } | ||
public DigestionParams DigestionParams { get; } | ||
public List<SilacLabel> SilacLabels { get; } | ||
// TODO: convert double range to MzRange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still need to do this?
@@ -79,14 +79,6 @@ public virtual string ToString(string format) | |||
return $"[{Minimum.ToString(format, System.Globalization.CultureInfo.InvariantCulture)};{Maximum.ToString(format, System.Globalization.CultureInfo.InvariantCulture)}]"; | |||
} | |||
|
|||
/// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intended deletion?
<file src="ThermoRawFileReader\bin\x64\Release\net6.0\ThermoFisher.CommonCore.Data.dll" target="lib\net6.0" /> | ||
<file src="ThermoRawFileReader\bin\x64\Release\net6.0\ThermoFisher.CommonCore.MassPrecisionEstimator.dll" target="lib\net6.0" /> | ||
<file src="ThermoRawFileReader\bin\x64\Release\net6.0\ThermoFisher.CommonCore.RawFileReader.dll" target="lib\net6.0" /> | ||
<file src="UsefulProteomicsDatabases\bin\x64\Release\net6.0\UsefulProteomicsDatabases.dll" target="lib\net6.0" /> | ||
<!--net6.0-windows target with mzPlot--> | ||
<file src="mzPlot\bin\x64\Release\net6.0-windows\mzPlot.dll" target="lib\net6.0-windows7.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This net6.0-windows
section has some net6.0
stuff in it
This is the minimal viable implementation of a new deconvolution method, spectral deconvolution.
Spectral Deconvolution works by generating theoretical isotopic distributions for each peptide/proteoform in a given library, generating isotopic envelopes for each charge state, then comparing the envelopes in each spectra to those in the library.
Note that this PR includes a significant refactor (see PR #671) that moves the Proteomics project into the MassSpectrometry project. Due to this change, loading this PR necessitates a clean and rebuild and/or a restart of visual studio.
I also added a Scorer class that implements the Strategy design pattern to score experimental spectra against theoretical spectra. I still need to write tests for this class.