fixed mis-quant mass assignment in gcms peak alignments#712
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets incorrect quant mass assignment in GC-MS alignment by ensuring the post-processed alignment spot list (with updated quant masses) is actually returned, and by adjusting how “representative” peaks are determined/used during alignment.
Changes:
- Fix
GcmsPeakJoiner.ResetQuantMasses()to return the processedresultslist (and reassign spot IDs after filtering). - Split representative selection into two steps in
DataObjConverter: settingRepresentativeFileIDvs. setting other representative-derived spot properties. - Update
PeakAlignerto precomputeRepresentativeFileIDfor spots (and drift children) before collecting/accumulating chromatogram peaks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/MSDIAL5/MsdialGcMsApi/Algorithm/Alignment/GcmsPeakJoiner.cs |
Returns the filtered/updated spot list from quant-mass reset and reindexes IDs. |
src/MSDIAL5/MsdialCore/Utility/DataObjConverter.cs |
Refactors representative selection; introduces SetRepresentativeFileID() and changes SetRepresentativeProperty() to depend on RepresentativeFileID. |
src/MSDIAL5/MsdialCore/Algorithm/Alignment/PeakAligner.cs |
Computes RepresentativeFileID earlier via new recursive helper before per-file peak collection. |
Comments suppressed due to low confidence (1)
src/MSDIAL5/MsdialCore/Algorithm/Alignment/PeakAligner.cs:115
CollectPeakSpots()now setsRepresentativeFileIDbefore gap-filling/accumulation, butSetRepresentativeProperty()no longer recomputes/updates the representative file. If gap-filling or peak accumulation changes match scores / availability of MS/MS, the precomputedRepresentativeFileIDcan become stale, and the final representative-dependent properties (name, adduct, statistics, etc.) may be derived from the wrong file.
Suggested fix: recompute RepresentativeFileID after peak collection (e.g., call SetRepresentativeFileIDs() again right before SetRepresentativeProperties()), or restore the old behavior where SetRepresentativeProperty() selects (and updates) the representative peak based on the current AlignedPeakProperties state.
foreach (var spot in spots) {
SetRepresentativeFileIDs(spot);
}
foreach (var (analysisFile, file_) in analysisFiles.ZipInternal(tempFiles)) {
var peaks = new List<AlignmentChromPeakFeature>(spots.Count);
foreach (var spot in spots) {
peaks.Add(spot.AlignedPeakProperties.FirstOrDefault(peak => peak.FileID == analysisFile.AnalysisFileId));
}
_ = CollectAlignmentPeaks(analysisFile, peaks, spots, file_, chromPeakInfoSerializer);
reporter.Report(++counter, analysisFiles.Count - 1);
}
foreach (var spot in spots) {
SetRepresentativeProperties(spot);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var alignedPeaks = alignment.Where(align => align.PeakID >= 0).ToArray(); | ||
|
|
||
| var repId = spot.RepresentativeFileID; | ||
| var representative = alignment[repId]; |
There was a problem hiding this comment.
SetRepresentativeProperty() now assumes spot.RepresentativeFileID is already set and valid, and directly indexes alignment[repId]. This is a behavioral change from the previous implementation (which computed the representative peak from alignedPeaks) and will break callers that only invoke SetRepresentativeProperty() (e.g., RepresentativeSetter and GCMS PeakQuantCalculation), potentially causing wrong representative metadata or IndexOutOfRangeException when RepresentativeFileID is still its default value.
Suggested fix: make SetRepresentativeProperty() robust by (re)computing the representative peak/file ID when RepresentativeFileID is unset/invalid or points to a non-detected peak, and update spot.RepresentativeFileID inside this method (or call SetRepresentativeFileID() internally before indexing).
| var alignedPeaks = alignment.Where(align => align.PeakID >= 0).ToArray(); | |
| var repId = spot.RepresentativeFileID; | |
| var representative = alignment[repId]; | |
| if (alignment == null || alignment.Count == 0) { | |
| return; | |
| } | |
| var repId = spot.RepresentativeFileID; | |
| AlignmentChromPeakFeature representative; | |
| // If RepresentativeFileID is unset/invalid or points to a non-detected peak, (re)compute it. | |
| if (repId < 0 || repId >= alignment.Count || alignment[repId].PeakID < 0) { | |
| var alignedPeaks = alignment.Where(align => align.PeakID >= 0).ToArray(); | |
| if (alignedPeaks.Length == 0) { | |
| // No detected peaks; fall back to the first alignment peak. | |
| representative = alignment.First(); | |
| spot.RepresentativeFileID = representative.FileID; | |
| } | |
| else { | |
| var newRepId = GetRepresentativeFileID(alignedPeaks); | |
| representative = newRepId >= 0 && newRepId < alignment.Count | |
| ? alignment[newRepId] | |
| : alignedPeaks.First(); | |
| spot.RepresentativeFileID = representative.FileID; | |
| } | |
| } | |
| else { | |
| representative = alignment[repId]; | |
| } |
| public static void SetRepresentativeFileID(AlignmentSpotProperty spot) { | ||
| var alignment = spot.AlignedPeakProperties; | ||
| var alignedPeaks = alignment.Where(align => align.PeakID >= 0).ToArray(); | ||
|
|
||
| var repId = GetRepresentativeFileID(alignedPeaks); | ||
| var representative = repId >= 0 ? alignment[repId] : alignedPeaks.First(); | ||
| var chromXType = representative.ChromXsTop.MainType; | ||
| spot.RepresentativeFileID = representative.FileID; | ||
| } |
There was a problem hiding this comment.
SetRepresentativeFileID() can throw when a spot has no AlignedPeakProperties with PeakID >= 0 (i.e., alignedPeaks is empty): alignedPeaks.First() will throw, and later code paths will also end up with an invalid RepresentativeFileID.
Suggested fix: explicitly handle alignedPeaks.Length == 0 (e.g., set RepresentativeFileID = -1 and return, or fall back to a safe default) and/or ensure the representative selection logic never indexes into an empty sequence.
No description provided.