Dotp indication in Prosit mirror plot (issue 831) #3193
Conversation
| var intensities2All = new List<double>(vectorLength); | ||
| var matchIndex1 = 0; | ||
| var matchIndex2 = 0; | ||
| while (matchIndex1 < spectrum1.Intensities.Count() && matchIndex2 < spectrum2.Intensities.Count()) |
There was a problem hiding this comment.
This .Intensities.Count() call is actually really inefficient. It would be better to use .Peaks.Count instead.
| /// <param name="mzTolerance">Tolerance for considering two mzs the same</param> | ||
| /// <param name="ignoreMismatches1">if true, unmatched peaks from the first spectrum are ignored</param> | ||
| /// <param name="ignoreMismatches2">if true, unmatched peaks from the second spectrum are ignored</param> | ||
| public static double CalculateSpectrumDotpMzFull(LibraryRankedSpectrumInfo spectrum1, |
There was a problem hiding this comment.
I think CalculateSpectrumDotpMzMatch should be changed to call CalculateSpectrumDotpMzFull passing in false for ignoreMismatches1 and ignoreMismatches2 so that there is less duplicated code.
I think the implementation in "CalculateSpectrumDotpMzMatch" is a lot cleaner in terms of iterating over the "RankedMI" lists in the two spectra. I think it would be better if "CalculateSpectrumDotpMzFull" looked more like the way "CalculateSpectrumDotpMzMatch" looks like now.
| { | ||
| var other = matched2.Where(m => m.MatchedIons.Intersect()) | ||
| switch (mzTolerance.CompareWithTolerance(s1[matchIndex1].Mz, s2[matchIndex2].Mz)) |
There was a problem hiding this comment.
I think the way that CalculateSpectrumDotpMzMatch does this with:
if (mzTolerance.IsWithinTolerance(mz1, mz2))
{
...
}
else if (mz1 < mz2)
{
...
}
else
{
...
}
is easier to understand than this way of doing a switch on the return value of CompareWithTolerance.
| @@ -85,6 +85,17 @@ public bool IsWithinTolerance(double a, double b) | |||
| return (a >= b - this) && (a <= b + this); | |||
| } | |||
|
|
|||
| public int CompareWithTolerance(double a, double b) | |||
There was a problem hiding this comment.
I would recommend removing this method because it's entirely obvious just from the method's name what exactly it's going to do.
If you were going to keep this method, I think its implementation should make use of IsWithinTolerance:
public int CompareWithTolerance(double a, double b)
{
if (IsWithinTolerance(a, b))
{
return 0;
}
return a < b ? -1 : 1;
}
|
|
||
| foreach (var match1 in matched1) | ||
| var vectorLength = spectrum1.Intensities.Count() + spectrum2.Intensities.Count(); | ||
| var s1 = Enumerable.Zip(spectrum1.MZs, spectrum1.Intensities, |
There was a problem hiding this comment.
This is almost the same as s1=spectrum1.Peaks.ToList(), except that the code here produces a List of MI whereas "Peaks" is a list of RankedMI.
| } | ||
| else if (mz1 < mz2) | ||
|
|
||
| if (mz1 < mz2) |
There was a problem hiding this comment.
I am pretty sure this needs to be "else if (mz1 < mz2)" because otherwise "matchIndex1" or "matchIndex2" will potentially get incremented twice.
https://skyline.ms/issues/home/issues/details.view?issueId=831