FLASHDeconv including a complete build of OpenMS 3.4.1#947
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR integrates FLASHDeconv deconvolution functionality into mzLib by adding executable runtime support, streamlining existing deconvolution tests, and implementing comprehensive integration testing for the FLASHDeconv tool from executable usage.
Key Changes:
- Adds FLASHDeconv executable runtime wrapper with robust options handling and error management
- Significantly refactors and simplifies existing deconvolution tests for better maintainability
- Adds comprehensive test coverage for FLASHDeconv integration with baseline comparison testing
Reviewed Changes
Copilot reviewed 12 out of 244 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TestDeconvolution.cs | Refactored deconvolution tests with simplified variable naming and consolidated test logic |
| Test.csproj | Added new test data files and simplified project configuration with consolidated file entries |
| FlashDeconvOptionsTests.cs | New comprehensive test class for FLASHDeconv integration with tolerance-based comparison |
| FlashDeconvRunner.cs | New runtime wrapper for FLASHDeconv executable with preflight validation and timeout handling |
| FlashDeconvOptions.cs | New strongly-typed options class for FLASHDeconv command-line arguments |
| MassSpectrometry.csproj | Added OpenMS/FLASHDeconv external runtime dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var dp = new IsoDecDeconvolutionParameters(); | ||
| var alg = new IsoDecAlgorithm(dp); | ||
| var allMasses = alg.Deconvolute(scan.MassSpectrum, | ||
| new MzRange((double)scan.MassSpectrum.FirstX!, (double)scan.MassSpectrum.LastX!)).ToList(); |
There was a problem hiding this comment.
The null-forgiving operators (!) are being used without null checks. Consider adding defensive null checks or documenting why these properties are guaranteed to be non-null in this context.
| bool any1 = mono1.Any(m => m >= 14037.926829 - ppmwidth && m <= 14037.926829 + ppmwidth); | ||
| bool any2 = mono2.Any(m => m >= 14037.926829 - ppmwidth && m <= 14037.926829 + ppmwidth); |
There was a problem hiding this comment.
The magic number 14037.926829 is repeated multiple times. Consider extracting this as a named constant like EXPECTED_MONOISOTOPIC_MASS to improve maintainability and reduce the risk of typos.
| _exe = FlashDeconvLocator.FindExecutable(overridePath) | ||
| ?? throw new FileNotFoundException("FLASHDeconv executable not found (env FLASHDECONV_PATH or vendored runtime)."); |
There was a problem hiding this comment.
The FlashDeconvLocator.FindExecutable method is referenced but not defined in the provided diff. This creates a dependency that may not be satisfied. Consider ensuring this dependency is properly implemented or document its expected interface.
| string input = GetValueAfter(list, "-in") ?? throw new InvalidOperationException("Missing -in in serialized args."); | ||
| string output = GetValueAfter(list, "-out") ?? throw new InvalidOperationException("Missing -out in serialized args."); |
There was a problem hiding this comment.
The error messages could be more helpful by including context about what caused the serialization to fail. Consider adding information about the validation state or expected format.
| string input = GetValueAfter(list, "-in") ?? throw new InvalidOperationException("Missing -in in serialized args."); | |
| string output = GetValueAfter(list, "-out") ?? throw new InvalidOperationException("Missing -out in serialized args."); | |
| string input = GetValueAfter(list, "-in") ?? throw new InvalidOperationException( | |
| $"Missing -in in serialized args. " + | |
| $"InputMzMlPath: '{opts.InputMzMlPath ?? "null"}'. " + | |
| $"Argument list: [{string.Join(", ", list)}]. " + | |
| "Expected format: -in <input file>." | |
| ); | |
| string output = GetValueAfter(list, "-out") ?? throw new InvalidOperationException( | |
| $"Missing -out in serialized args. " + | |
| $"OutputMzMlPath: '{opts.OutputMzMlPath ?? "null"}'. " + | |
| $"Argument list: [{string.Join(", ", list)}]. " + | |
| "Expected format: -out <output file>." | |
| ); |
trishorts
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #947 +/- ##
==========================================
- Coverage 80.94% 79.80% -1.14%
==========================================
Files 265 270 +5
Lines 38331 38807 +476
Branches 4138 4208 +70
==========================================
- Hits 31026 30969 -57
- Misses 6607 7137 +530
- Partials 698 701 +3
🚀 New features to boost your workflow:
|
|
Closing — superseded by #1045, which resolves a user-installed FLASHDeconv.exe instead of vendoring the OpenMS 3.4.1 binaries. |
This pull request introduces a fully instrumented integration layer for the external FLASHDeconv executable, adding: (1) a minimalist algorithm wrapper (FlashDeconv) that resolves the binary and executes it with robust diagnostic error handling; (2) a discovery + preflight utility (FlashDeconvLocator) to locate the executable via override path, environment variable, or conventional bundled location and optionally validate dependency loading; (3) a higher‑level FlashDeconvRunner with optional preflight, timeout and async execution support; (4) a strongly typed FlashDeconvOptions model (with nested groups) that mirrors the complete CLI surface and safely serializes to argument tokens; and (5) rich XML-style and rationale-focused inline documentation throughout clarifying both what each component does and why specific design choices were made (fail‑fast validation, separation of concerns, explicit argument emission, deferred parsing). Together these changes centralize process orchestration, improve configurability and debuggability, and establish a foundation for future result parsing without coupling core deconvolution abstractions to evolving output formats.
This code was generated by creating a local build of https://github.com/OpenMS/OpenMS version 3.4.1
A reference for this work can be found here:
Pfeuffer, J., Bielow, C., Wein, S. et al. OpenMS 3 enables reproducible analysis of large-scale mass spectrometry data, Nat Methods 21, 365–367 (2024). https://doi.org/10.1038/s41592-024-02197-7