Initial implementation of quant mass manger for gcms project#657
Initial implementation of quant mass manger for gcms project#657
Conversation
This commit introduces two new static methods to the `ChromatogramRange` class: `FromTimes<T>(T center, double width)` and `FromTimes<T>(T begin, T end)`. The `FromTimes<T>(T center, double width)` method allows for creating a `ChromatogramRange` based on a center point and a specified width, while the `FromTimes<T>(T begin, T end)` method facilitates the creation of a `ChromatogramRange` using two defined endpoints.
Updated `GetMs1ExtractedChromatogram` to use a new method `GetMs1ExtractedChromatogram_temp2` for improved retrieval of MS1 extracted chromatograms. Added an overloaded method that accepts an `MzRange` object, enhancing the handling of mass-to-charge ratio and tolerance parameters.
Introduced the `SearchNearestPoint` method in `Chromatogram.cs` to enhance peak searching functionality with improved handling of null peak lists. An overloaded version was also added for specific chromatogram types. Refactored peak finding logic in `LcmsGapFiller.cs` to utilize the new `SearchNearestPoint` method, simplifying the code and improving readability. Adjusted `minId` handling to default to the middle of the chromatogram when no nearest peak is found.
Introduced `SearchLeftEdge`, `SearchRightEdge`, `SearchLeftEdgeHard`, and `SearchRightEdgeHard` methods in the `CompMs.Common.Components` namespace to enhance peak boundary detection based on intensity comparisons. Updated the `SearchNearestPoint` method to call a private overload with `ChromXs`. Refactored the `LcmsGapFiller` class to utilize the new edge search methods, simplifying the logic for determining left and right indices around peaks. Updated peak top determination to use `IntensityDifference` for improved accuracy.
- Removed IReadOnlyList<RawSpectrum> parameter from AccumulateChromatogram in multiple classes to simplify method signatures and improve internal data handling. - Updated PeakAligner.cs to reflect the new method signature by removing the spectrum argument. - Added GetRepresentativeSpectrum method to Ms1Spectra for enhanced functionality in retrieving representative spectra. - Optimized GetMSScanProperties methods to return lists directly from the chromatogram variable, improving performance. - Adjusted using directives across several files for better organization and streamlined dependencies. - Enhanced comments and documentation in the GetRepresentativeSpectrum method for improved readability and maintainability.
Introduce a static readonly instance of `ReportProgress` named `_empty` to streamline object creation when `reportAction` or `progress` is `null`. Update the `FromLength` and `FromRange` methods to return this instance instead of creating new `Progress<int>` objects, reducing unnecessary allocations and improving code consistency. These changes enhance readability and maintainability across multiple method overloads.
Introduces the `PeakQuantCalculation` class in the `CompMs.MsdialGcMsApi.Algorithm` namespace. This class facilitates asynchronous peak quantification calculations, collects alignment peaks, and serializes spot information. It leverages various data structures and algorithms from the `CompMs` library, including temporary file handling and progress reporting.
- Introduced `CreatePeakQuantCalculation` method in `GcmsAlignmentProcessFactory` for peak quantification. - Added properties in `AlignmentFileBeanModel` to improve access to alignment file details. - Updated `GcmsAlignmentModel` to accept `PeakQuantCalculation` for direct utilization in `QuantmassBrowserModel`. - Modified `QuantmassBrowserModel` constructor to integrate quantification features. - Enhanced `MainWindow.xaml.cs` to provide a "Run" action for quantification in the UI. - Implemented `FinishCommand` in `QuantmassBrowserViewModel` to trigger quantification asynchronously.
Updated AlignmentFileBeanModel to include EicFilePath for retrieving the EIC file path. Removed CreateEicLoader method and replaced its usage with direct instantiation of AlignmentEicLoader across multiple files. Adjusted AlignmentEicLoader constructor to accept AlignmentFileBeanModel, modifying internal logic to utilize the new EicFilePath property.
Changed the `SelectedItem` binding in the `DataGrid` from
`{Binding Path=SelectedSpot.Value}` to `{Binding Path=Spots/}`.
This updates the selection logic to bind directly to the
`Spots` collection.
This commit introduces a loop that iterates over the collection of `spots` and invokes the `SetRepresentativeProperty` method from the `DataObjConverter` class for each `spot`. This enhancement ensures that each `spot` has its representative property set after collecting alignment peaks and reporting progress.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the gap-filling functionality in MS-DIAL, introducing a new IGapFiller interface and significantly refactoring the GC-MS and LC-MS gap-filling implementations. It also adds a new "Quant mass browser" feature for GC-MS alignment and removes unused parameters from method signatures across multiple data accessor classes.
- Introduced
IGapFillerinterface to standardize gap-filling implementations - Refactored
LcmsGapFillerandGcmsGapFillerto implement the new interface with standalone logic - Added a quantification mass browser UI feature for GC-MS projects
- Removed the unused
spectrumparameter fromAccumulateChromatogrammethods across all data accessors
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
IGapFiller.cs |
New interface defining the gap-filling contract |
GapFiller.cs |
Updated base class to implement IGapFiller |
LcmsGapFiller.cs |
Complete refactor implementing standalone gap-filling logic |
GcmsGapFiller.cs |
Refactored to implement IGapFiller with independent gap-filling implementation |
DataAccessor.cs and implementations |
Removed unused spectrum parameter from AccumulateChromatogram |
AlignmentSpotPropertyModel.cs |
Added QuantMass property with change notification |
QuantmassBrowserModel.cs |
New model for quant mass browser feature |
QuantmassBrowserView.xaml |
New UI for browsing and editing quantification masses |
Chromatogram.cs |
Added utility methods for peak detection and edge finding |
ChromatogramRange.cs |
Added FromTimes overload for center/width construction |
Ms1Spectra.cs |
Added GetRepresentativeSpectrum and GetMs1ExtractedChromatogram overload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| d:DesignHeight="450" d:DesignWidth="800"> | ||
| <Grid> | ||
| <DataGrid ItemsSource="{Binding Path=Spots}" | ||
| SelectedItem="{Binding Path=Spots/}" |
There was a problem hiding this comment.
The binding path Spots/ is incomplete. This should be SelectedItem=\"{Binding Path=SelectedSpot}\" to properly bind to the ViewModel's SelectedSpot property.
| var peaklist = ms1Spectra.GetMs1ExtractedChromatogram(center.Mz.Value, this.mzTol, chromatogramRange); | ||
| return peaklist.ChromatogramSmoothing(smoothingMethod, smoothingLevel).AsPeakArray(); | ||
| int id = nearestTopId; | ||
| for(int i = leftId + 1; i <= rightId - 1; i++) { |
There was a problem hiding this comment.
Missing space after for keyword. Should be for (int i = leftId + 1; i <= rightId - 1; i++) to follow C# coding conventions.
|
|
||
| if (!isForceInsert && (id - leftId < 2 || rightId - id < 2)) return (-1, -1, -1); | ||
|
|
||
| for(int i = leftId + 1; i <= rightId - 1; i++) { |
There was a problem hiding this comment.
Missing space after for keyword. Should be for (int i = leftId + 1; i <= rightId - 1; i++) to follow C# coding conventions.
| for(int i = leftId + 1; i <= rightId - 1; i++) { | |
| for (int i = leftId + 1; i <= rightId - 1; i++) { |
| .Select(i => _spectra[i]) | ||
| .Where(spectrum => spectrum.MsLevel == 2) | ||
| .Where(spectrum => spectrum.ScanPolarity == scanPolarity) | ||
| .Where(spectrum => spectrum.Precursor.ContainsMz(rtPeakFeature.PeakFeature.Mass, ms2Tolerance, acquisitionType)) // mz is in range |
There was a problem hiding this comment.
Property access changed from rtPeakFeature.Mass to rtPeakFeature.PeakFeature.Mass. Verify that ChromatogramPeakFeature has a PeakFeature property with a Mass member, as this could cause a null reference exception if the property path is incorrect.
| .Where(spectrum => spectrum.Precursor.ContainsMz(rtPeakFeature.PeakFeature.Mass, ms2Tolerance, acquisitionType)) // mz is in range | |
| .Where(spectrum => rtPeakFeature.PeakFeature != null && spectrum.Precursor.ContainsMz(rtPeakFeature.PeakFeature.Mass, ms2Tolerance, acquisitionType)) // mz is in range |
| public interface IGapFiller | ||
| { | ||
| bool NeedsGapFill(AlignmentSpotProperty spot, AnalysisFileBean analysisFile); | ||
| void GapFill(Ms1Spectra ms1Spectra, RawSpectra rawSpectra, IReadOnlyList<RawSpectrum> spectra, AlignmentSpotProperty spot, int fileID); |
There was a problem hiding this comment.
The IGapFiller interface includes both RawSpectra and IReadOnlyList<RawSpectrum> parameters, which appears redundant since RawSpectra internally contains the spectrum collection. Consider removing the spectra parameter if it's not needed by all implementations.
| void GapFill(Ms1Spectra ms1Spectra, RawSpectra rawSpectra, IReadOnlyList<RawSpectrum> spectra, AlignmentSpotProperty spot, int fileID); | |
| void GapFill(Ms1Spectra ms1Spectra, RawSpectra rawSpectra, AlignmentSpotProperty spot, int fileID); |
| } | ||
|
|
||
| public AlignmentSpotPropertyModelCollection? Spots => _spots; | ||
| public AlignmentSpotPropertyModelCollection Spots => _spots ?? throw new ObjectDisposedException(nameof(Spots)); |
There was a problem hiding this comment.
Throwing ObjectDisposedException with nameof(Spots) is misleading because Spots is the property name, not the disposed object. Should use nameof(AlignmentSpotSource) or a more descriptive message to indicate the entire source has been disposed.
| public AlignmentSpotPropertyModelCollection Spots => _spots ?? throw new ObjectDisposedException(nameof(Spots)); | |
| public AlignmentSpotPropertyModelCollection Spots => _spots ?? throw new ObjectDisposedException(nameof(AlignmentSpotSource)); |
| return topId >= 1 && topId < _size - 1 | ||
| && _peaks[topId - 1].Intensity <= _peaks[topId].Intensity | ||
| && _peaks[topId].Intensity >= _peaks[topId + 1].Intensity; | ||
| && _peaks[topId].Intensity >= _peaks[topId + 1].Intensity |
There was a problem hiding this comment.
The additional conditions for IsPeakTop (lines 575-576) exclude flat peaks where all three consecutive points have equal intensity. While this may be intentional, this logic change affects peak detection throughout the system and should be clearly documented or have a comment explaining why flat peaks are excluded.
| && _peaks[topId].Intensity >= _peaks[topId + 1].Intensity | |
| && _peaks[topId].Intensity >= _peaks[topId + 1].Intensity | |
| // Exclude flat peaks: if all three consecutive points have equal intensity, do not consider as a peak top. |
| } | ||
| } | ||
|
|
||
| private readonly static ReportProgress _empty = new(null, 0d, 100d); |
There was a problem hiding this comment.
The modifier order should be private static readonly instead of private readonly static to follow C# conventions (access modifier, then static, then readonly).
| private readonly static ReportProgress _empty = new(null, 0d, 100d); | |
| private static readonly ReportProgress _empty = new(null, 0d, 100d); |
No description provided.