[OpenSwathWorkflow] Feature Automate iRT Calibration#8146
Conversation
…t instead of passing a filepath
… to perform automatic iRT extraction by sampling the input targeted experiment
…and flag them as advanced
WalkthroughAdds on-the-fly iRT sampling, a new OpenSwathHelper::sampleExperiment, changes performCalibration to accept preloaded iRT transitions, refactors TOPPOpenSwathWorkflow to namespace Calibration options and support linear + optional nonlinear two-stage calibration, and updates tests to the new parameter namespace and auto_irt flag. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OpenSwathWorkflow
participant OpenSwathHelper
participant TOPPOpenSwathBase
User->>OpenSwathWorkflow: Run with transitions + Calibration params
OpenSwathWorkflow->>OpenSwathWorkflow: Resolve iRT sources (files or auto_irt)
alt auto_irt and no iRT files
OpenSwathWorkflow->>OpenSwathHelper: sampleExperiment(main transitions, bins, peptides/bin, seed, sort/top_fraction)
OpenSwathHelper-->>OpenSwathWorkflow: LightTargetedExperiment (linear iRT)
opt nonlinear configured
OpenSwathWorkflow->>OpenSwathHelper: sampleExperiment(...nonlinear params...)
OpenSwathHelper-->>OpenSwathWorkflow: LightTargetedExperiment (nonlinear iRT)
end
else user-provided iRT files
OpenSwathWorkflow->>OpenSwathWorkflow: Load iRT transitions from files
end
OpenSwathWorkflow->>TOPPOpenSwathBase: performCalibration(irt_transitions=linear)
TOPPOpenSwathBase-->>OpenSwathWorkflow: Linear RT transformation
opt nonlinear iRT present
OpenSwathWorkflow->>TOPPOpenSwathBase: performCalibration(irt_transitions=nonlinear)
TOPPOpenSwathBase-->>OpenSwathWorkflow: Nonlinear RT/IM corrections
OpenSwathWorkflow->>OpenSwathWorkflow: Invert & apply IM correction to library compounds
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
singjc
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/topp/OpenSwathWorkflow.cpp (1)
791-794: Use logical AND operator instead of bitwise AND.The condition uses bitwise AND (&) which will still work but is semantically incorrect for boolean logic.
Apply this diff:
- if ( (cp_irt.im_extraction_window == -1) & (cp.im_extraction_window != -1) ) + if ( (cp_irt.im_extraction_window == -1) && (cp.im_extraction_window != -1) )
🧹 Nitpick comments (2)
CHANGELOG (1)
44-45: Mention the new CLI flags for easier discoverabilityThe bullet correctly records the feature, but many users search the changelog for option names rather than narrative text. Consider appending the primary new flag (and a wildcard hint for the rest) so
grep/Ctrl-Ffinds it instantly:- - Added automated iRT calibration using input transition list (#8146) + - Added automated iRT calibration using input transition list (#8146, new flags: `-auto_irt`, `-Calibration:irt_*`)src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (1)
169-281: Consider refactoring this method to reduce complexity.The static analysis correctly identifies this method as complex. While the current implementation is functional and well-structured, consider breaking it down into smaller helper methods for better maintainability:
filterCandidates()for initial filteringsortAndTrimByIntensity()for intensity-based selectionsampleFromRTBins()for the RT binning logicassembleOutputExperiment()for constructing the resultWould you like me to provide a refactored version that breaks down this method into smaller, more focused functions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG(1 hunks)src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h(1 hunks)src/openms/include/OpenMS/APPLICATIONS/OpenSwathBase.h(3 hunks)src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp(3 hunks)src/topp/OpenSwathWorkflow.cpp(5 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
[notice] 169-281: src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp#L169-L281
Complex Method
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (macos-14, xcode, 15.0.1)
- GitHub Check: build-and-test (ubuntu-22.04, clang++, 15)
- GitHub Check: build-and-test (ubuntu-22.04, g++, 11)
- GitHub Check: build-and-test (macos-13, xcode, 14.2)
- GitHub Check: build-and-test (windows-2022, cl.exe, 14.36)
🔇 Additional comments (5)
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h (1)
184-209: LGTM! Well-documented method for automated iRT calibration.The method signature and documentation are comprehensive. The parameters provide good flexibility for different sampling strategies (filtering decoys, sorting by intensity, selecting top fraction).
src/openms/include/OpenMS/APPLICATIONS/OpenSwathBase.h (1)
333-334: Good architectural improvement for iRT transition handling.The change from file path to
LightTargetedExperimentobject makes the API more flexible and aligns well with the automated iRT sampling feature. The documentation updates accurately reflect the new parameter.Also applies to: 352-352, 379-385
src/topp/OpenSwathWorkflow.cpp (3)
408-421: Good addition of string validation for boolean options.The explicit validation of
auto_irt,enable_ms1, andenable_ipfto only accept "true" or "false" values improves input validation and user experience.
963-1008: Well-structured iRT experiment preparation logic.The code properly handles the precedence of iRT sources:
- User-supplied files take precedence
- Falls back to auto-sampling when enabled
- Clear logging messages for transparency
The sampling parameters for linear (40% top intensity) and nonlinear (70% top intensity) calibration are reasonable defaults.
1089-1094: Verify the ion mobility transformation logic.The code inverts the ion mobility transformation and applies it back to the compounds. Please ensure this is the intended behavior - typically calibrations are applied in the forward direction.
Would you like me to trace through the calibration workflow to verify this transformation direction is correct?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (2)
236-252: Optimize bin boundary handling for edge case.The last bin uses
rt_maxas the upper bound while others use< hi. This could cause issues if a compound has exactlyrt_maxretention time.Apply this diff for consistent boundary handling:
for (Size b = 0; b < bins; ++b) { double lo = rt_min + b * bin_width; - double hi = (b + 1 == bins ? rt_max : lo + bin_width); + double hi = lo + bin_width; std::vector<OpenSwath::LightCompound> bucket; for (auto & cmp : candidates) { - if (cmp.rt >= lo && cmp.rt < hi) + if (cmp.rt >= lo && (b + 1 == bins ? cmp.rt <= hi : cmp.rt < hi)) bucket.push_back(cmp); }
168-280: Consider refactoring this complex method for better maintainability.The method implements multiple distinct operations and is flagged as complex by static analysis. Consider breaking it into smaller, focused methods.
Consider extracting helper methods:
filterCandidatesByDecoys()(lines 179-197)sortAndTrimByIntensity()(lines 199-220)sampleAcrossRTBins()(lines 222-252)assembleOutputExperiment()(lines 254-279)This would improve readability, testability, and maintainability while preserving the current functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
[notice] 169-281: src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp#L169-L281
Complex Method
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (macos-14, xcode, 15.0.1)
- GitHub Check: build-and-test (ubuntu-22.04, clang++, 15)
- GitHub Check: build-and-test (macos-13, xcode, 14.2)
- GitHub Check: build-and-test (windows-2022, cl.exe, 14.36)
- GitHub Check: build-and-test (ubuntu-22.04, g++, 11)
🔇 Additional comments (2)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (2)
10-12: LGTM! Includes are necessary for the new sampling functionality.The new includes are properly justified:
<random>for RNG operations,<algorithm>for sorting/shuffling, and<unordered_set>for efficient lookups.
206-210: Intensity accumulation logic is now correct.The intensity accumulation logic has been fixed since the previous review. The condition now correctly accumulates intensities for all transitions that pass the decoy filter, resolving the issue where existing peptide references were being skipped.
| OpenSwath::LightTargetedExperiment | ||
| OpenSwathHelper::sampleExperiment( | ||
| const OpenSwath::LightTargetedExperiment & exp, | ||
| Size bins, | ||
| Size peptides_per_bin, | ||
| unsigned int seed, | ||
| bool filter_decoys, | ||
| bool sort_by_intensity, | ||
| double top_fraction) | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation for robustness.
The method lacks validation for critical parameters that could cause runtime issues.
Add parameter validation at the method start:
+ {
+ if (bins == 0)
+ throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "bins must be > 0");
+ if (peptides_per_bin == 0)
+ throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "peptides_per_bin must be > 0");
+ if (top_fraction < 0.0 || top_fraction > 1.0)
+ throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "top_fraction must be in [0.0, 1.0]");
+ if (exp.getCompounds().empty())
+ throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "experiment contains no compounds");
+
{Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp around lines 168 to
177, the sampleExperiment method lacks validation for critical input parameters.
Add checks at the beginning of the method to ensure parameters like bins,
peptides_per_bin, and top_fraction are within valid ranges (e.g., bins and
peptides_per_bin should be greater than zero, top_fraction should be between 0
and 1). If any parameter is invalid, handle it appropriately by throwing an
exception or returning an error to prevent runtime issues.
|
nice. how important is it to adapt the parameters? e.g. on short gradients? or sparse data? would be awesome if the defaults would be robust to different scenarios. |
Based on the prelim tests I've done so far, the defaults seem to be reasonable. I'm running some tests on a short gradient and sparse single cell dataset to see if the current defaults still work for those. Will update and let you know once I have those results. |
Below is testing the automated iRTs for calibration using the default settings for number of bins and number of peptides per bin for linear and additional non-linear calibration, compared to using user-specific provided iRT files. File 1: Regular DIA 120 min gradient (PASS01508, S. pyo)
File 2: diaPASEF Sparse Single-Cell (30min gradient)Note: Transition library was sampled for 10K random target peptides from a human library. Using the full library seemed to take too long to run on my local machine.
File 3: diaPASEF Short Gradient (200 SPD, ~5min gradient)Note: Transition library was sampled for 10K random target peptides from a library provided by @jcharkow . Using the full library seemed to take too long to run on my local machine.
|
|
Great! Good to go from my side but maybe good to have a review by @jcharkow |
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Thanks for taking on this long awaited feature for OpenSWATH!
I have a few comments/concerns above, mainly I am a bit confused how the intensity parameter will work when intensities in the library are normalized.
…boost::mt19937_64
…used for linear and nonlinear iRT sampling
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/topp/OpenSwathWorkflow.cpp (2)
470-470: Wrong parameter referenced insetMinFloat_
setMinFloat_("irt_mz_extraction_window", -1.0)is duplicated.
It should constrain the newly introducedirt_nonlinear_rt_extraction_window, otherwise that option is left without range checking.- setMinFloat_("irt_mz_extraction_window", -1.0); // means extract over the whole range + setMinFloat_("irt_nonlinear_rt_extraction_window", -1.0); // means extract over the whole range
790-794: Bit-wise ‘&’ used instead of logical ‘&&’
if ( (cp_irt.im_extraction_window == -1) & (cp.im_extraction_window != -1) )
&on bools works but is easy to mis-read and bypasses short-circuiting.
Replace with the logical operator:- if ( (cp_irt.im_extraction_window == -1) & (cp.im_extraction_window != -1) ) + if ( (cp_irt.im_extraction_window == -1) && (cp.im_extraction_window != -1) )
🧹 Nitpick comments (1)
src/topp/OpenSwathWorkflow.cpp (1)
408-412: Use a boolean flag instead of a string for-auto_irt
auto_irtis a pure on/off switch but is registered as aStringoption that requires the literal value “true”/“false”. UseregisterFlag_()(andgetFlag_()) for a simpler UX and to match the rest of TOPP tools.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/topp/OpenSwathWorkflow.cpp(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/include/OpenMS/**/*.h : Add Doxygen comments for new public methods and classes
Applied to files:
src/topp/OpenSwathWorkflow.cpp
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Applied to files:
src/topp/OpenSwathWorkflow.cpp
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Maintain consistency with the surrounding code
Applied to files:
src/topp/OpenSwathWorkflow.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (macos-14, xcode, 15.0.1)
- GitHub Check: build-and-test (ubuntu-22.04, g++, 11)
- GitHub Check: build-and-test (windows-2022, cl.exe, 14.36)
- GitHub Check: build-and-test (ubuntu-22.04, clang++, 15)
- GitHub Check: build-and-test (macos-13, xcode, 14.2)
| registerStringOption_("enable_ms1", "<true|false>", "true", "Extract the precursor ion trace(s) and use for scoring if present", false, true); | ||
| setValidStrings_("enable_ms1", ListUtils::create<String>("true,false")); | ||
|
|
||
| registerStringOption_("enable_ipf", "<name>", "true", "Enable additional scoring of identification assays using IPF (see online documentation)", false, true); | ||
| registerStringOption_("enable_ipf", "<true|false>", "true", "Enable additional scoring of identification assays using IPF (see online documentation)", false, true); | ||
| setValidStrings_("enable_ipf", ListUtils::create<String>("true,false")); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
enable_ms1 / enable_ipf should stay flags for backward-compatibility
These options were formerly boolean flags. Converting them to string parameters breaks existing command-lines that used “-enable_ms1” without a value. Unless you need tri-state behaviour, revert to registerFlag_ / getFlag_.
Revert enable_ms1 / enable_ipf to boolean flags for backward compatibility
Converting these two-state settings to string options forces users to write -enable_ms1 true (or false) and breaks all existing invocations that relied on bare -enable_ms1. Since you only need on/off behavior, revert to registerFlag_/getFlag_.
• In src/topp/OpenSwathWorkflow.cpp (≈ lines 416–421):
– Replace
cpp registerStringOption_("enable_ms1", "<true|false>", "true", "...", false, true); setValidStrings_("enable_ms1", ListUtils::create<String>("true,false")); registerStringOption_("enable_ipf", "<true|false>", "true", "...", false, true); setValidStrings_("enable_ipf", ListUtils::create<String>("true,false"));
with
cpp registerFlag_("enable_ms1", "Extract the precursor ion trace(s) and use for scoring if present"); registerFlag_("enable_ipf", "Enable additional scoring of identification assays using IPF (see online documentation)");
• Where these options are consumed, switch any getStringOption_("enable_ms1") / getStringOption_("enable_ipf") calls back to getFlag_("…").
🤖 Prompt for AI Agents
In src/topp/OpenSwathWorkflow.cpp around lines 416 to 421, revert the options
enable_ms1 and enable_ipf from string options back to boolean flags to maintain
backward compatibility. Replace the registerStringOption_ and setValidStrings_
calls for these options with registerFlag_ calls using the original
descriptions. Additionally, update any code that reads these options to use
getFlag_ instead of getStringOption_ to reflect the boolean flag usage.
| // ----------------------------------------------------------------- | ||
| // Validate auto_irt parameters | ||
| // ----------------------------------------------------------------- | ||
| if (auto_irt) | ||
| { | ||
| // linear sampling must have at least one bin and one peptide per bin | ||
| if (irt_bins_lin == 0) | ||
| { | ||
| writeLogError_("Parameter error: --irt_bins must be > 0 when auto_irt is enabled."); | ||
| return PARSE_ERROR; | ||
| } | ||
| if (irt_pep_lin == 0) | ||
| { | ||
| writeLogError_("Parameter error: --irt_peptides_per_bin must be > 0 when auto_irt is enabled."); | ||
| return PARSE_ERROR; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extend validation to non-linear sampling parameters
When auto_irt is enabled and irt_peptides_per_bin_nonlinear > 0, irt_bins_nonlinear should also be validated to be > 0 (mirroring the linear checks) to prevent division-by-zero in the sampler.
🤖 Prompt for AI Agents
In src/topp/OpenSwathWorkflow.cpp around lines 740 to 757, extend the validation
logic for auto_irt parameters to include non-linear sampling. Specifically, when
auto_irt is enabled and irt_peptides_per_bin_nonlinear is greater than 0, add a
check to ensure irt_bins_nonlinear is also greater than 0. If irt_bins_nonlinear
is 0 or less, log an error message similar to the linear case and return
PARSE_ERROR to prevent division-by-zero errors.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/topp/OpenSwathWorkflow.cpp (1)
469-471: Wrong parameter min bound is set (typo): enables negative m/z window.setMinFloat_ on irt_mz_extraction_window is set to -1.0 with a comment about “whole range,” which applies to RT, not m/z. This would allow negative m/z windows.
Apply this diff:
- setMinFloat_("irt_mz_extraction_window", -1.0); // means extract over the whole range + setMinFloat_("irt_nonlinear_rt_extraction_window", -1.0); // -1 means extract over the whole RT range
♻️ Duplicate comments (4)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (2)
216-225: Guard against empty candidates and invalid RT range.When all peptides are filtered out, rt_min = +inf and rt_max = -inf, leading to negative/NaN bin_width. Bail out early with a clear error.
Apply this diff:
// 2) estimate RT range using filtered candidates double rt_min = std::numeric_limits<double>::max(); double rt_max = std::numeric_limits<double>::lowest(); for (auto & cmp : candidates) { rt_min = std::min(rt_min, cmp.rt); rt_max = std::max(rt_max, cmp.rt); } + if (candidates.empty()) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, + "No non-decoy candidates available for sampling (empty experiment or all filtered out)."); + } + if (!(rt_max > rt_min)) + { + // all RTs are equal or invalid range; force a single, zero-width bin + bins = 1; + } double bin_width = (rt_max - rt_min) / static_cast<double>(bins);
169-176: Add parameter validation to prevent undefined behavior.Validate bins, peptides_per_bin, top_fraction, and that the input contains compounds. Throwing InvalidParameter is consistent with OpenMS error handling.
Apply this diff:
OpenSwathHelper::sampleExperiment( const OpenSwath::LightTargetedExperiment & exp, Size bins, Size peptides_per_bin, unsigned int seed, bool sort_by_intensity, double top_fraction) { + if (bins == 0) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "bins must be > 0"); + } + if (peptides_per_bin == 0) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "peptides_per_bin must be > 0"); + } + if (top_fraction < 0.0 || top_fraction > 1.0) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "top_fraction must be in [0.0, 1.0]"); + } + if (exp.getCompounds().empty()) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "experiment contains no compounds"); + }src/topp/OpenSwathWorkflow.cpp (2)
416-421: Revert enable_ms1/enable_ipf to flags for backward compatibility.These options historically were flags. Keeping them as string options breaks existing scripts that pass -enable_ms1 without a value.
Apply this diff:
- registerStringOption_("enable_ms1", "<true|false>", "true", "Extract the precursor ion trace(s) and use for scoring if present", false, true); - setValidStrings_("enable_ms1", ListUtils::create<String>("true,false")); + registerFlag_("enable_ms1", "Extract the precursor ion trace(s) and use for scoring if present", true); @@ - registerStringOption_("enable_ipf", "<true|false>", "true", "Enable additional scoring of identification assays using IPF (see online documentation)", false, true); - setValidStrings_("enable_ipf", ListUtils::create<String>("true,false")); + registerFlag_("enable_ipf", "Enable additional scoring of identification assays using IPF (see online documentation)", true);Additionally, update reads (outside this hunk):
// change bool use_ms1_traces = getStringOption_("enable_ms1") == "true"; bool enable_uis_scoring = getStringOption_("enable_ipf") == "true"; // to bool use_ms1_traces = getFlag_("enable_ms1"); bool enable_uis_scoring = getFlag_("enable_ipf");
740-756: Extend validation to non-linear sampling to avoid division-by-zero.When auto_irt is enabled and irt_peptides_per_bin_nonlinear > 0, ensure irt_bins_nonlinear > 0 as well.
Apply this diff:
if (auto_irt) { // linear sampling must have at least one bin and one peptide per bin if (irt_bins_lin == 0) @@ return PARSE_ERROR; } + // validate nonlinear sampling if enabled + if (irt_pep_nl > 0 && irt_bins_nl == 0) + { + writeLogError_("Parameter error: --irt_bins_nonlinear must be > 0 when irt_peptides_per_bin_nonlinear > 0."); + return PARSE_ERROR; + } }
🧹 Nitpick comments (4)
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h (1)
184-201: Doxygen: tighten wording and reflect decoy behavior.Shorten and clarify comments, and reflect that decoys are always excluded.
Apply this diff:
- * Splits the RT span (min→max) into @p bins and randomly picks up to - * @p peptides_per_bin compounds from each bin. Useful for on-the-fly - * iRT calibration without external .irt files. + * Splits the RT span (min→max) into @p bins and randomly picks up to + * @p peptides_per_bin non-decoy compounds from each bin. Useful for on-the-fly + * iRT calibration without external .irt files. @@ - * @param[in] sort_by_intensity Whether to sort the assays by the highest cumulative intense transitions. This is useful for sampling the most intense peptides for iRTs. - * @param[in] top_fraction Only sample from the top N fraction of sorted assays to narrow down on only really intense peptides. This is useful for selecting a few "high quality" peptides to use for linear iRTs. + * @param[in] sort_by_intensity Whether to rank assays by cumulative transition intensities per peptide before sampling. + * @param[in] top_fraction Fraction [0.0,1.0] of top-ranked assays to retain before sampling.src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (2)
263-272: Use std::string for protein ids to match LightCompound::protein_refs.Consistent key type avoids conversions.
Apply this diff:
- std::unordered_set<String> prot_ids; + std::unordered_set<std::string> prot_ids;
169-281: Consider splitting sampleExperiment into smaller helpers.The method’s complexity was flagged. Extract “rankCandidatesByIntensity_” and “sampleByRtBins_” as private helpers in the .cpp to improve readability and testability.
src/topp/OpenSwathWorkflow.cpp (1)
991-1006: Comment/implementation mismatch: 80% vs 70%.The comment says “top 80%” but the code passes 0.7 (70%). Align the comment or adjust the constant. Given previous discussion, updating the comment is sufficient.
Apply this diff:
- // Note2: For the additional nonlinear iRTs we set top fraction to 80%, that is, we reduce the space of peptides to sample for 80% of the highest intense peptides. + // Note2: For the additional nonlinear iRTs we set top fraction to 70%, that is, we reduce the space of peptides to sample for 70% of the highest-intensity peptides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h(1 hunks)src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp(3 hunks)src/topp/OpenSwathWorkflow.cpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/openms/**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
src/openms/**/*.{cpp,h,hpp,cc,cxx}: Follow the existing C++ coding conventions in the codebase
Use the established naming patterns for classes, methods, and variables
Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Follow the established error handling patterns
Utilize OpenMS logging mechanisms
Be mindful of memory usage when processing large datasets
Consider algorithmic complexity for data processing operations
Use appropriate OpenMS containers and algorithms
Files:
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.hsrc/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
src/openms/include/OpenMS/**/*.h
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Add Doxygen comments for new public methods and classes
Files:
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h
🧠 Learnings (3)
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use appropriate OpenMS containers and algorithms
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
📚 Learning: 2025-04-07T21:16:06.557Z
Learnt from: jpfeuffer
PR: OpenMS/OpenMS#7970
File: src/pyOpenMS/addons/MSSpectrum.pyx:24-28
Timestamp: 2025-04-07T21:16:06.557Z
Learning: In OpenMS, the proper way to access ion mobility data in MSSpectrum is to use the `getIMData()` method, which returns a tuple with (position, unit). The position corresponds to the index in the FloatDataArrays where the ion mobility data is stored. Always use this method rather than assuming the first FloatDataArray contains ion mobility data.
Applied to files:
src/topp/OpenSwathWorkflow.cpp
🪛 GitHub Check: CodeFactor
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
[notice] 169-281: src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp#L169-L281
Complex Method
🔇 Additional comments (5)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (3)
193-215: Remove undefined filter_decoys and always exclude decoys; fix intensity accumulation container type.filter_decoys is not a parameter here and is undefined. Exclude decoys unconditionally. Also use std::unordered_map<std::string, double> for consistency.
Apply this diff:
- if (sort_by_intensity && top_fraction > 0.0 && top_fraction <= 1.0) + if (sort_by_intensity && top_fraction > 0.0 && top_fraction <= 1.0) { // sum intensities per peptide across all transitions - std::unordered_map<String, double> intensity_sum; + std::unordered_map<std::string, double> intensity_sum; for (auto & tr : exp.getTransitions()) { - if (!filter_decoys || !tr.decoy) - { - intensity_sum[tr.getPeptideRef()] += tr.library_intensity; - } + if (!tr.decoy) + { + intensity_sum[tr.getPeptideRef()] += tr.library_intensity; + } }Likely an incorrect or invalid review comment.
253-261: Clean up decoy filtering and use std::string for ids.Remove filter_decoys usage (undefined) and exclude decoys unconditionally. Use std::unordered_setstd::string for pep_ids.
Apply this diff:
- std::unordered_set<String> pep_ids; + std::unordered_set<std::string> pep_ids; for (auto & cmp : picked) pep_ids.insert(cmp.id); for (auto & tr : exp.getTransitions()) { - if (pep_ids.count(tr.getPeptideRef()) && (!filter_decoys || !tr.decoy)) + if (pep_ids.count(tr.getPeptideRef()) && !tr.decoy) out_exp.transitions.push_back(tr); }Likely an incorrect or invalid review comment.
181-191: Use std::string keys in unordered containers to avoid potential hashing issues.OpenSwath Light types use std::string for ids. Using OpenMS::String in unordered containers requires a std::hash specialization. Use std::string to be safe and consistent.
Apply this diff:
- std::unordered_set<String> good_ids; + std::unordered_set<std::string> good_ids; @@ - if (good_ids.count(cmp.id)) + if (good_ids.count(cmp.id)) candidates.push_back(cmp);⛔ Skipped due to learnings
Learnt from: CR PR: OpenMS/OpenMS#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-08-05T12:43:11.681Z Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use appropriate OpenMS containers and algorithmssrc/topp/OpenSwathWorkflow.cpp (2)
963-979: Harden inline docs: magic constant rationale is fine; ensure arguments are correct with updated signature.After fixing the header, these calls will bind correctly to (seed, sort_by_intensity, top_fraction). No code change needed here.
946-952: INI subsection parameter copy: trailing colon consistency.You copy "Calibration:RTNormalization:" (with trailing colon) but "Calibration:MassIMCorrection" (without colon). This is subtle but consistent with other tools expecting no colon for non-nested subsection. If intentional, ignore; otherwise, standardize.
Do you want both to use a trailing colon? If yes, change to:
- getParam_().copy("Calibration:MassIMCorrection:", true)
| #include <random> | ||
| #include <algorithm> | ||
| #include <unordered_set> | ||
|
|
There was a problem hiding this comment.
Missing includes cause build failures.
Math::RandomShuffler and std::unordered_map are used but their headers are not included.
Apply this diff:
#include <random>
#include <algorithm>
#include <unordered_set>
+#include <unordered_map>
+#include <OpenMS/MATH/MathFunctions.h>🤖 Prompt for AI Agents
In src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp around lines 11-14,
the file uses Math::RandomShuffler and std::unordered_map but does not include
their headers, causing build failures; add the appropriate includes (the OpenMS
RandomShuffler header — e.g. OpenMS/MATH/RandomShuffler.h or the correct project
path for Math::RandomShuffler) and the standard <unordered_map> header near the
other includes so the types are declared and the file compiles.
| for (Size b = 0; b < bins; ++b) | ||
| { | ||
| double lo = rt_min + b * bin_width; | ||
| double hi = (b + 1 == bins ? rt_max : lo + bin_width); | ||
| std::vector<OpenSwath::LightCompound> bucket; | ||
| for (auto & cmp : candidates) | ||
| { | ||
| if (cmp.rt >= lo && cmp.rt < hi) | ||
| bucket.push_back(cmp); | ||
| } | ||
| if (!bucket.empty()) | ||
| { | ||
| rshuffler.portable_random_shuffle(bucket.begin(), bucket.end()); | ||
| Size take = std::min(peptides_per_bin, bucket.size()); | ||
| picked.insert(picked.end(), bucket.begin(), bucket.begin() + take); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid RT endpoints falling through cracks; include upper edge in last bin.
With the current < hi check, compounds at rt_max are dropped. Make the last bin upper bound inclusive.
Apply this diff:
for (Size b = 0; b < bins; ++b)
{
double lo = rt_min + b * bin_width;
double hi = (b + 1 == bins ? rt_max : lo + bin_width);
std::vector<OpenSwath::LightCompound> bucket;
for (auto & cmp : candidates)
{
- if (cmp.rt >= lo && cmp.rt < hi)
+ const bool last_bin = (b + 1 == bins);
+ if (cmp.rt >= lo && (last_bin ? (cmp.rt <= hi) : (cmp.rt < hi)))
bucket.push_back(cmp);
}
if (!bucket.empty())
{
rshuffler.portable_random_shuffle(bucket.begin(), bucket.end());
Size take = std::min(peptides_per_bin, bucket.size());
picked.insert(picked.end(), bucket.begin(), bucket.begin() + take);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (Size b = 0; b < bins; ++b) | |
| { | |
| double lo = rt_min + b * bin_width; | |
| double hi = (b + 1 == bins ? rt_max : lo + bin_width); | |
| std::vector<OpenSwath::LightCompound> bucket; | |
| for (auto & cmp : candidates) | |
| { | |
| if (cmp.rt >= lo && cmp.rt < hi) | |
| bucket.push_back(cmp); | |
| } | |
| if (!bucket.empty()) | |
| { | |
| rshuffler.portable_random_shuffle(bucket.begin(), bucket.end()); | |
| Size take = std::min(peptides_per_bin, bucket.size()); | |
| picked.insert(picked.end(), bucket.begin(), bucket.begin() + take); | |
| } | |
| } | |
| for (Size b = 0; b < bins; ++b) | |
| { | |
| double lo = rt_min + b * bin_width; | |
| double hi = (b + 1 == bins ? rt_max : lo + bin_width); | |
| std::vector<OpenSwath::LightCompound> bucket; | |
| for (auto & cmp : candidates) | |
| { | |
| const bool last_bin = (b + 1 == bins); | |
| if (cmp.rt >= lo && (last_bin ? (cmp.rt <= hi) : (cmp.rt < hi))) | |
| bucket.push_back(cmp); | |
| } | |
| if (!bucket.empty()) | |
| { | |
| rshuffler.portable_random_shuffle(bucket.begin(), bucket.end()); | |
| Size take = std::min(peptides_per_bin, bucket.size()); | |
| picked.insert(picked.end(), bucket.begin(), bucket.begin() + take); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp around lines
231-247, the current rt binning uses cmp.rt >= lo && cmp.rt < hi so any compound
with rt == rt_max falls through; update the membership check for the last bin to
include the upper edge by changing the condition to allow cmp.rt <= hi when
b+1==bins (e.g. compute hi as before but when comparing use (cmp.rt >= lo &&
(cmp.rt < hi || (b+1==bins && cmp.rt <= hi)))). Ensure no other logic relies on
strict < for non-last bins.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h (1)
184-201: Clarify Doxygen: decoy exclusion, parameter ranges, and avoid non-ASCII glyphs.Tighten the docs to state that decoys are always excluded, define expected parameter ranges, clarify top_fraction behavior, and replace Unicode arrow/hyphen to avoid Doxygen encoding issues.
Apply this diff:
- /** - * @brief Sample a subset of peptides uniformly across the RT range. - * - * Splits the RT span (min→max) into @p bins and randomly picks up to - * @p peptides_per_bin compounds from each bin. Useful for on-the-fly - * iRT calibration without external .irt files. - * - * @param[in] exp Full LightTargetedExperiment (the input peptide query parameter assay list for targeted extraction) - * @param[in] bins Number of retention‐time bins (i.e. 10 bins across the RT range for linear iRT, 500-1000 bins across the RT for nonlinear iRT) - * @param[in] peptides_per_bin How many peptides to draw per bin (i.e. 5 peptides for linear iRT, 25 - 50 for non-linear iRT) - * @param[in] seed If non‐zero, used to seed the RNG (deterministic). - * If zero, will use std::random_device for non-deterministic. - * @param[in] sort_by_intensity Whether to sort the assays by the highest cumulative intense transitions. This is useful for sampling the most intense peptides for iRTs. - * @param[in] top_fraction Only sample from the top N fraction of sorted assays to narrow down on only really intense peptides. This is useful for selecting a few "high quality" peptides to use for linear iRTs. - * - * @return A new LightTargetedExperiment containing only the sampled - * compounds, their transitions, and associated proteins. - */ + /** + * @brief Sample a subset of target peptides uniformly across the RT range. + * + * Splits the RT span (min->max) into @p bins and randomly picks up to + * @p peptides_per_bin compounds from each bin. Decoy assays are always + * excluded from sampling. Useful for on-the-fly iRT calibration without + * external iRT files. + * + * @param[in] exp Full LightTargetedExperiment (the input peptide query parameter assay list for targeted extraction). + * @param[in] bins Number of retention time bins (e.g., ~10 for linear iRT, 500-1000 for nonlinear iRT). Must be >= 1. + * @param[in] peptides_per_bin Peptides to draw per bin (e.g., ~5 for linear iRT, 25-50 for nonlinear iRT). Must be >= 1. + * @param[in] seed RNG seed. If non-zero, sampling is deterministic; if zero, a non-deterministic std::random_device is used. + * @param[in] sort_by_intensity If true, assays are sorted by cumulative transition intensity before sampling. + * @param[in] top_fraction Only sample from the top fraction of the sorted assays (0 < top_fraction <= 1). Ignored if @p sort_by_intensity is false. + * + * @return A new LightTargetedExperiment containing only the sampled + * compounds, their transitions, and associated proteins. + * + * @note Requires valid RT annotations on the input assays; entries without RT are ignored. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/openms/**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
src/openms/**/*.{cpp,h,hpp,cc,cxx}: Follow the existing C++ coding conventions in the codebase
Use the established naming patterns for classes, methods, and variables
Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Follow the established error handling patterns
Utilize OpenMS logging mechanisms
Be mindful of memory usage when processing large datasets
Consider algorithmic complexity for data processing operations
Use appropriate OpenMS containers and algorithms
Files:
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h
src/openms/include/OpenMS/**/*.h
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Add Doxygen comments for new public methods and classes
Files:
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (ubuntu-22.04, g++, 11)
- GitHub Check: build-and-test (windows-2022, cl.exe, 14.36)
- GitHub Check: build-and-test (ubuntu-22.04, clang++, 15)
- GitHub Check: build-and-test (macos-14, xcode, 15.0.1)
- GitHub Check: build-and-test (macos-13, xcode, 14.2)
🔇 Additional comments (2)
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h (2)
202-209: Signature alignment looks good; decoy flag removal resolved prior mismatch.The declaration now matches the implementation intent (no filter_decoys arg), preventing misbinding and linker issues mentioned earlier. Thanks for simplifying the API.
206-207: Seed default inconsistency — callers pass seed; no change requiredCLI default (irt_seed) = 5489, API default in sampleExperiment = 0 (non‑deterministic). I verified all call sites explicitly pass the seed, so the API default being 0 is safe.
- Header: src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h — sampleExperiment(..., unsigned int seed = 0, ...)
- Workflow: src/topp/OpenSwathWorkflow.cpp — p.setValue("irt_seed", 5489) (defaults), UInt irt_seed = irt_calibration_params.getValue("irt_seed"), and calls sampleExperiment(..., irt_seed, ...) (around lines ~573, ~676, ~975, ~1002).
Conclusion: callers supply the seed; no change required to the API default.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (4)
11-14: Missing required header includes will cause build failures.The code uses
std::unordered_mapandMath::RandomShufflerbut doesn't include their headers. Add the missing includes to ensure compilation.Apply this diff:
#include <random> #include <algorithm> #include <unordered_set> +#include <unordered_map> +#include <OpenMS/MATH/MathFunctions.h>
169-177: Add parameter validation for robustness.The method lacks validation for critical parameters that could cause runtime issues or unexpected behavior.
Add parameter validation at the beginning of the method:
OpenSwathHelper::sampleExperiment( const OpenSwath::LightTargetedExperiment & exp, Size bins, Size peptides_per_bin, unsigned int seed, bool sort_by_intensity, double top_fraction) { + // Validate input parameters + if (bins == 0) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, + "bins must be > 0"); + } + if (peptides_per_bin == 0) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, + "peptides_per_bin must be > 0"); + } + if (sort_by_intensity && (top_fraction <= 0.0 || top_fraction > 1.0)) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, + "top_fraction must be in (0.0, 1.0] when sort_by_intensity is enabled"); + } + if (exp.getCompounds().empty()) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, + "experiment contains no compounds"); + } + // 0) initial candidate selection: exclude decoys
216-224: Handle empty candidates to prevent invalid RT range calculations.If all peptides are filtered out,
rt_minwill remain at+∞andrt_maxat-∞, causing negativebin_widthand undefined behavior in downstream operations.Add validation after computing the RT range:
double rt_min = std::numeric_limits<double>::max(); double rt_max = std::numeric_limits<double>::lowest(); for (auto & cmp : candidates) { rt_min = std::min(rt_min, cmp.rt); rt_max = std::max(rt_max, cmp.rt); } + + // Handle case where no candidates remain after filtering + if (candidates.empty() || rt_min >= rt_max) + { + // Return empty experiment when no valid candidates + return OpenSwath::LightTargetedExperiment(); + } + double bin_width = (rt_max - rt_min) / static_cast<double>(bins);
231-240: Include upper RT edge in the last bin to avoid dropping peptides.The current binning logic uses
cmp.rt < hifor all bins, which excludes compounds withrt == rt_max. The last bin should include the upper boundary to capture all peptides.Apply this diff to include the upper edge in the last bin:
for (Size b = 0; b < bins; ++b) { double lo = rt_min + b * bin_width; double hi = (b + 1 == bins ? rt_max : lo + bin_width); std::vector<OpenSwath::LightCompound> bucket; + const bool last_bin = (b + 1 == bins); for (auto & cmp : candidates) { - if (cmp.rt >= lo && cmp.rt < hi) + if (cmp.rt >= lo && (last_bin ? (cmp.rt <= hi) : (cmp.rt < hi))) bucket.push_back(cmp); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/openms/**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
src/openms/**/*.{cpp,h,hpp,cc,cxx}: Follow the existing C++ coding conventions in the codebase
Use the established naming patterns for classes, methods, and variables
Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Follow the established error handling patterns
Utilize OpenMS logging mechanisms
Be mindful of memory usage when processing large datasets
Consider algorithmic complexity for data processing operations
Use appropriate OpenMS containers and algorithms
Files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
🧠 Learnings (2)
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use appropriate OpenMS containers and algorithms
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
🧬 Code Graph Analysis (1)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (2)
src/openms/include/OpenMS/MATH/MathFunctions.h (1)
seed(495-498)src/openms/source/FORMAT/OSWFile.cpp (1)
tr(647-651)
🪛 GitHub Check: CodeFactor
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
[notice] 169-281: src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp#L169-L281
Complex Method
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (ubuntu-22.04, clang++, 15)
- GitHub Check: build-and-test (macos-13, xcode, 14.2)
- GitHub Check: build-and-test (ubuntu-22.04, g++, 11)
- GitHub Check: build-and-test (macos-14, xcode, 15.0.1)
- GitHub Check: build-and-test (windows-2022, cl.exe, 14.36)
🔇 Additional comments (3)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (3)
179-191: LGTM! Clear and efficient decoy filtering logic.The approach of first collecting non-decoy transition IDs and then filtering compounds based on those IDs is clean and performant.
194-214: LGTM! Well-implemented intensity-based sorting and filtering.The intensity aggregation and sorting logic correctly sums library intensities per peptide and efficiently filters to the top fraction of candidates.
249-273: LGTM! Clean assembly of the output experiment.The code correctly assembles the sampled experiment by:
- Including only the selected compounds
- Filtering transitions to match selected peptides (excluding decoys)
- Including only referenced proteins
The use of unordered_sets for lookups is efficient.
| Math::RandomShuffler rshuffler; | ||
| rshuffler.seed(seed == 0 ? std::random_device{}() : seed); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify RandomShuffler seed compatibility.
The Math::RandomShuffler::seed() method expects uint64_t (based on the codebase context), but the function parameter is unsigned int. This type mismatch could cause issues on platforms where these types differ.
Consider using consistent types or adding an explicit cast if needed:
- rshuffler.seed(seed == 0 ? std::random_device{}() : seed);
+ rshuffler.seed(seed == 0 ? std::random_device{}() : static_cast<uint64_t>(seed));🏁 Script executed:
#!/bin/bash
# Description: Check the exact signature of Math::RandomShuffler::seed() method
# Find the RandomShuffler class definition and seed method
ast-grep --pattern 'class RandomShuffler {
$$$
seed($_) {
$$$
}
$$$
}'
# Also check for the seed method signature more broadly
rg -A 2 "void seed\(" --glob "**/*RandomShuffler*" --glob "**/MathFunctions.h"Length of output: 362
Fix RandomShuffler seed type mismatch
Math::RandomShuffler::seed expects a uint64_t (verified: src/openms/include/OpenMS/MATH/MathFunctions.h: void seed(uint64_t val)), but the call passes an unsigned int parameter. Cast or align types to avoid platform-dependent issues.
- Locations to update:
- src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp — lines ~229-230
- src/openms/include/OpenMS/MATH/MathFunctions.h —
void seed(uint64_t val)(verified)
Suggested change:
- Math::RandomShuffler rshuffler;
- rshuffler.seed(seed == 0 ? std::random_device{}() : seed);
+ Math::RandomShuffler rshuffler;
+ rshuffler.seed(seed == 0 ? std::random_device{}() : static_cast<uint64_t>(seed));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Math::RandomShuffler rshuffler; | |
| rshuffler.seed(seed == 0 ? std::random_device{}() : seed); | |
| Math::RandomShuffler rshuffler; | |
| rshuffler.seed(seed == 0 ? std::random_device{}() : static_cast<uint64_t>(seed)); |
🤖 Prompt for AI Agents
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp around lines 229-230:
the call to Math::RandomShuffler::seed passes an unsigned int (and
std::random_device::operator() result) but the method expects a uint64_t; fix by
ensuring the ternary expression yields a uint64_t (cast the random_device result
and the seed value to uint64_t or cast the whole expression with
static_cast<uint64_t>) so the call matches seed(uint64_t). No change to
MathFunctions.h is required.
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Looks good now, just some minor comments. The main one being is there an option still to do no calibration.
I am still a bit confused by the intensity filtering (see #8146 (comment)) but if it works than it works.
After these comments are addressed it is ready to merge on my end.
|
|
||
| #include <OpenMS/ANALYSIS/OPENSWATH/OpenSwathHelper.h> | ||
|
|
||
| #include <random> |
There was a problem hiding this comment.
is random still needed since I believe you migrated to using boost now?
There was a problem hiding this comment.
Random is still needed for setting a random seed when not using an explicit seed.
| p.setValue("tr_irt_nonlinear", "", "additional nonlinear transition file ('TraML'). Takes precedent even when `auto_rt` is set to 'true'"); | ||
|
|
||
| p.setValue("rt_norm", "", "RT normalization file (how to map the RTs of this run to the ones stored in the library). If set, tr_irt may be omitted."); | ||
|
|
There was a problem hiding this comment.
Did you preserve the option to skip calibration? This option is useful if we have a library with experimental RT values and do not need calibration.
Previously, if no irt files were supplied, then calibration would be skipped. I am not sure if this is preserved.
Possibly we need another parameter such as "perform_calibration" now that iRTs are optional.
There was a problem hiding this comment.
The option to skip calibration is still preserved. if auto_irt is set to false, and no irt files or rt_norm file is provided, then calibration is skipped.
There was a problem hiding this comment.
This might not be obvious. Would it make sense to add this to the Description?
There was a problem hiding this comment.
This might not be obvious. Would it make sense to add this to the Description?
Kk, I updated the description to make it clear that when auto_irt is set to false and no iRT files or trafoXML files are provided, then calibration is not performed.
…arameters and ensure sufficient candidates for iRT calibration
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (3)
202-210: Intensity accumulation logic fixed (no longer skips existing peptides)This addresses the earlier issue where intensities weren’t accumulated for multiple transitions per peptide. Good use of a sum over non-decoy transitions.
11-14: Missing headers for RandomShuffler, unordered_map, and numeric_limits — compilation will failThis file uses Math::RandomShuffler (requires OpenMS/MATH/MathFunctions.h), std::unordered_map, and std::numeric_limits but doesn't include the corresponding headers.
Apply this diff to fix the includes:
#include <random> #include <algorithm> #include <unordered_set> +#include <unordered_map> +#include <limits> +#include <OpenMS/MATH/MathFunctions.h>
245-252: Include upper edge in the last RT bin to avoid dropping rt_max compoundsWith the current < hi condition, compounds at rt_max may be dropped (especially when bin_width == 0). Make the last bin upper bound inclusive.
Apply this diff:
double lo = rt_min + b * bin_width; double hi = (b + 1 == bins ? rt_max : lo + bin_width); std::vector<OpenSwath::LightCompound> bucket; for (auto & cmp : candidates) { - if (cmp.rt >= lo && cmp.rt < hi) + const bool last_bin = (b + 1 == bins); + if (cmp.rt >= lo && (last_bin ? (cmp.rt <= hi) : (cmp.rt < hi))) bucket.push_back(cmp); }
🧹 Nitpick comments (6)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (6)
229-237: Handle equal-RT edge case explicitly to avoid empty samplingWhen rt_min == rt_max (e.g., highly filtered input), bin_width becomes 0. Without special handling, all buckets will be empty and yield no picks. We guard against this by including the rt_max boundary in the last bin (see next comment), but it’s also fine to short-circuit to place all into the last bin if desired.
If you want an explicit guard (optional):
double bin_width = (rt_max - rt_min) / static_cast<double>(bins); +// optional: if all RTs are equal, keep bin_width=0 and let the last-bin-inclusive check collect them +// alternatively, you could special-case here to avoid scanning all bins.
241-244: Cast seed to uint64_t to match RandomShuffler::seed signatureMath::RandomShuffler::seed expects uint64_t. Cast the ternary expression to avoid platform-dependent narrowing.
Apply this diff:
- rshuffler.seed(seed == 0 ? std::random_device{}() : seed); + rshuffler.seed(static_cast<uint64_t>(seed == 0 ? std::random_device{}() : seed));Note: Keeping is justified as std::random_device is used for non-deterministic seeding.
243-259: Optional: reduce sampling complexity from O(N*bins) to O(N) by pre-bucketingCurrent nested scan iterates all candidates for each bin. For large libraries and high bin counts (e.g., nonlinear defaults), this is avoidable.
Sketch within the same block (replace the nested scan with pre-bucketing):
- for (Size b = 0; b < bins; ++b) - { - double lo = rt_min + b * bin_width; - double hi = (b + 1 == bins ? rt_max : lo + bin_width); - std::vector<OpenSwath::LightCompound> bucket; - for (auto & cmp : candidates) - { - const bool last_bin = (b + 1 == bins); - if (cmp.rt >= lo && (last_bin ? (cmp.rt <= hi) : (cmp.rt < hi))) - bucket.push_back(cmp); - } - if (!bucket.empty()) - { - rshuffler.portable_random_shuffle(bucket.begin(), bucket.end()); - Size take = std::min(peptides_per_bin, bucket.size()); - picked.insert(picked.end(), bucket.begin(), bucket.begin() + take); - } - } + // pre-bucket in O(N) + std::vector<std::vector<OpenSwath::LightCompound>> buckets(bins); + for (const auto& cmp : candidates) + { + Size idx = 0; + if (bin_width > 0.0) + { + const double pos = (cmp.rt - rt_min) / bin_width; + idx = std::min<Size>(bins - 1, static_cast<Size>(pos)); + } + // if bin_width == 0, everything goes to last bin (idx defaults to 0; put them into last bin) + if (bin_width == 0.0) idx = bins - 1; + buckets[idx].push_back(cmp); + } + for (Size b = 0; b < bins; ++b) + { + auto& bucket = buckets[b]; + if (!bucket.empty()) + { + rshuffler.portable_random_shuffle(bucket.begin(), bucket.end()); + const Size take = std::min(peptides_per_bin, bucket.size()); + picked.insert(picked.end(), bucket.begin(), bucket.begin() + take); + } + }
265-273: Clarify comment: decoys are always excluded hereThe implementation unconditionally excludes decoys. Update the comment to avoid confusion.
- // copy matching transitions, excluding decoys if requested + // copy matching transitions, excluding decoys
169-281: Split sampleExperiment into smaller helpers to reduce complexity and improve testabilityStatic analysis flags this as a complex method. Consider extracting logical steps into helper functions (e.g., buildCandidates, computeIntensitySums, sortAndTrim, computeRtRange, sampleBins, assembleOutExperiment).
314-315: Remove stray blank linesMinor formatting nit to stay consistent with the file’s style.
- -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/openms/**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
src/openms/**/*.{cpp,h,hpp,cc,cxx}: Follow the existing C++ coding conventions in the codebase
Use the established naming patterns for classes, methods, and variables
Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Follow the established error handling patterns
Utilize OpenMS logging mechanisms
Be mindful of memory usage when processing large datasets
Consider algorithmic complexity for data processing operations
Use appropriate OpenMS containers and algorithms
Files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
🧠 Learnings (3)
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
📚 Learning: 2025-08-14T10:19:54.298Z
Learnt from: jpfeuffer
PR: OpenMS/OpenMS#8167
File: src/topp/Digestor.cpp:200-226
Timestamp: 2025-08-14T10:19:54.298Z
Learning: In OpenMS (C++ bioinformatics framework), use boost random libraries instead of std library random distributions for cross-platform reproducible random number generation, as std library distributions are not reproducible across platforms.
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use appropriate OpenMS containers and algorithms
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
🪛 GitHub Check: CodeFactor
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
[notice] 169-281: src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp#L169-L281
Complex Method
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (macos-13, xcode, 14.2)
- GitHub Check: build-and-test (ubuntu-22.04, clang++, 15)
- GitHub Check: build-and-test (ubuntu-22.04, g++, 11)
- GitHub Check: build-and-test (macos-14, xcode, 15.0.1)
- GitHub Check: build-and-test (windows-2022, cl.exe, 14.36)
🔇 Additional comments (3)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (3)
169-177: Good addition: focused, reusable sampling utility with sensible defaultsThe public signature and defaults look consistent with the PR objectives, and the method lives in the right helper.
178-183: Parameter validation is on pointPreconditions correctly constrain bins, peptides_per_bin, and top_fraction when sorting is enabled.
186-196: Candidate set construction is correct and minimalFiltering by non-decoy peptide refs and then selecting matching compounds is appropriate and efficient.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (4)
198-219: Intensity accumulation logic is correct and addresses earlier bugSumming intensities per peptide across non-decoy transitions ensures proper ranking. This matches the earlier fix to always accumulate and not skip existing keys.
243-259: Off-by-one in RT binning drops rt_max compounds and empties when bin_width == 0The condition cmp.rt >= lo && cmp.rt < hi excludes exact rt_max, and when all RTs are identical (bin_width == 0), no item is selected. Include the upper bound in the last bin.
Apply this diff:
- for (Size b = 0; b < bins; ++b) + for (Size b = 0; b < bins; ++b) { - double lo = rt_min + b * bin_width; - double hi = (b + 1 == bins ? rt_max : lo + bin_width); + double lo = rt_min + b * bin_width; + const bool last_bin = (b + 1 == bins); + double hi = last_bin ? rt_max : (lo + bin_width); std::vector<OpenSwath::LightCompound> bucket; for (auto & cmp : candidates) { - if (cmp.rt >= lo && cmp.rt < hi) + if (cmp.rt >= lo && (last_bin ? (cmp.rt <= hi) : (cmp.rt < hi))) bucket.push_back(cmp); }
11-14: Missing headers for RandomShuffler, unordered_map, and numeric_limits — will fail to compile on some setupsThis file uses Math::RandomShuffler, std::unordered_map, and std::numeric_limits, but their headers are not included locally. Do not rely on transitive includes; add the explicit headers.
Apply this diff:
#include <random> #include <algorithm> #include <unordered_set> +#include <unordered_map> +#include <limits> +#include <OpenMS/MATH/MathFunctions.h>
178-182: Promote preconditions to runtime validation (release builds) with proper exceptionsOPENMS_PRECONDITION only checks in debug builds. Add explicit runtime validation to protect against invalid parameters in release builds and align with OpenMS error handling.
Apply this diff:
{ - OPENMS_PRECONDITION(bins >= 1, "bins must be >= 1"); - OPENMS_PRECONDITION(peptides_per_bin >= 1, "peptides_per_bin must be >= 1"); - OPENMS_PRECONDITION(!sort_by_intensity || (top_fraction > 0.0 && top_fraction <= 1.0), - "top_fraction must be in (0,1] when sort_by_intensity is true"); + OPENMS_PRECONDITION(bins >= 1, "bins must be >= 1"); + OPENMS_PRECONDITION(peptides_per_bin >= 1, "peptides_per_bin must be >= 1"); + OPENMS_PRECONDITION(!sort_by_intensity || (top_fraction > 0.0 && top_fraction <= 1.0), + "top_fraction must be in (0,1] when sort_by_intensity is true"); + + if (bins == 0) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "bins must be > 0"); + } + if (peptides_per_bin == 0) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "peptides_per_bin must be > 0"); + } + if (sort_by_intensity && (top_fraction <= 0.0 || top_fraction > 1.0)) + { + throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "top_fraction must be in (0,1]"); + } + if (exp.getCompounds().empty()) + { + throw Exception::IllegalArgument(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "Experiment contains no compounds"); + }
🧹 Nitpick comments (2)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (2)
241-243: Seed type safety: cast to uint64_t to match RandomShuffler::seed signatureMath::RandomShuffler::seed expects uint64_t. Cast both branches of the ternary to avoid platform-dependent implicit conversions.
Apply this diff:
- rshuffler.seed(seed == 0 ? std::random_device{}() : seed); + rshuffler.seed(seed == 0 + ? static_cast<uint64_t>(std::random_device{}()) + : static_cast<uint64_t>(seed));
238-259: Potential O(bins × N) hot path; consider pre-sorting by RT to reduce complexityFor large bins (e.g., nonlinear stage default 2000) and large libraries, scanning all candidates per bin is costly. Pre-sort candidates by RT once and sweep with moving indices to populate buckets in O(N + bins) time.
Happy to sketch a refactor if you want to take this optimization in this PR or a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/openms/**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
src/openms/**/*.{cpp,h,hpp,cc,cxx}: Follow the existing C++ coding conventions in the codebase
Use the established naming patterns for classes, methods, and variables
Use OpenMS data structures (e.g., MSExperiment, FeatureMap, PeptideIdentification)
Follow the established error handling patterns
Utilize OpenMS logging mechanisms
Be mindful of memory usage when processing large datasets
Consider algorithmic complexity for data processing operations
Use appropriate OpenMS containers and algorithms
Files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
🧠 Learnings (2)
📚 Learning: 2025-08-14T10:19:54.298Z
Learnt from: jpfeuffer
PR: OpenMS/OpenMS#8167
File: src/topp/Digestor.cpp:200-226
Timestamp: 2025-08-14T10:19:54.298Z
Learning: In OpenMS (C++ bioinformatics framework), use boost random libraries instead of std library random distributions for cross-platform reproducible random number generation, as std library distributions are not reproducible across platforms.
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
📚 Learning: 2025-08-05T12:43:11.681Z
Learnt from: CR
PR: OpenMS/OpenMS#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Applies to src/openms/**/*.{cpp,h,hpp,cc,cxx} : Use appropriate OpenMS containers and algorithms
Applied to files:
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
🧬 Code Graph Analysis (1)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp (2)
src/openms/include/OpenMS/MATH/MathFunctions.h (1)
seed(495-498)src/openms/source/CONCEPT/Exception.cpp (1)
IllegalArgument(247-250)
🪛 GitHub Check: CodeFactor
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp
[notice] 169-281: src/openms/source/ANALYSIS/OPENSWATH/OpenSwathHelper.cpp#L169-L281
Complex Method
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (windows-2022, cl.exe, 14.36)
- GitHub Check: build-and-test (ubuntu-22.04, g++, 11)
- GitHub Check: build-and-test (ubuntu-22.04, clang++, 15)
- GitHub Check: build-and-test (macos-14, xcode, 15.0.1)
- GitHub Check: build-and-test (macos-13, xcode, 14.2)
|
@timosachsenberg @jcharkow I think this is ready to merge? |
|
Yes good to merge on my end |
timosachsenberg
left a comment
timosachsenberg
left a comment
There was a problem hiding this comment.
Thanks! Nice contribution
4 participants
Description
Added option for automated iRT calibration using the input transition list. This is useful if known standard iRTs (Biognosys kit) were not spiked-in. This also makes it easier for the user to not have to worry about where/how to generate these extra iRT files (which is usually generated from the transition list, if using EasyPQP).
Added the following params:
Benchmark on 1 run of the Gold Standard S. pyogenes dataset
Providing an iRT file (Biognosys iRTs) Linear Alignment
Automatic iRT Linear Alignment
Automatic iRT Linear and Nonlinear Alignment
Checklist
How can I get additional information on failed tests during CI
Click to expand
If your PR is failing you can check outIf you click in the column that lists the failed tests you will get detailed error messages.
Advanced commands (admins / reviewer only)
Click to expand
/reformat(experimental) applies the clang-format style changes as additional commit. Note: your branch must have a different name (e.g., yourrepo:feature/XYZ) than the receiving branch (e.g., OpenMS:develop). Otherwise, reformat fails to push.rebuild jenkinswill retrigger Jenkins-based CI buildsSummary by CodeRabbit
New Features
Changes
Tests
Documentation