Refactor/osw calibration#8743
Merged
Merged
Conversation
…hWorkflow - Introduced CalibrationWorkflow for modular calibration of RT, m/z, and ion mobility. - Replaced manual calibration logic with a single call to CalibrationWorkflow, simplifying the calibration process. - Added support for loading existing RT transformation files, skipping calibration if provided. - Updated configuration mapping from TOPP parameters to CalibrationConfig for better parameter management. - Enhanced logging for clarity during calibration steps and outcomes. - Included new header file CalibrationWorkflow.h in OpenSwathWorkflow.cpp.
- Removed the dependency on a transformation file for calibration, allowing for a more flexible approach using iRT experiments. - Introduced a new IrtExperiments structure to manage linear and nonlinear iRT data. - Added a strategy for preparing iRT experiments based on static files or sampling from a full transition library. - Updated performCalibration, performLinearCalibration, and performLinearThenNonlinearCalibration methods to utilize the new iRT structure and parameters. - Enhanced logging for better traceability during calibration processes. - Removed redundant validation functions and streamlined configuration checks. - Improved handling of estimated extraction windows based on new parameters.
…s for iRT calibration
…CalibrationWorkflow and update related parameters
…nd OpenSwathWorkflow
… CalibrationWorkflow
…in CalibrationWorkflow and SwathMapMassCorrection
…on in CalibrationWorkflow and SwathMapMassCorrection
…on and RTNormalization settings
…e fresh copies for each run, preventing parameter carry-over.
… info level and add progress indicators for calibration steps
…ng RT transformation and remove unnecessary info logs
… info level for calibration steps
…setters and use CalibrationWorkflow for RT normalization
… centralize RT/m/z normalization logic in CalibrationWorkflow
…and nonlinear calibration steps
…del parameters to match original implementation
…ion logic and improve code clarity
…od descriptions in CalibrationWorkflow
… detailed chromatogram simulations
…test for multi mzml and multi linear iRT input
Contributor
📝 WalkthroughWalkthroughRefactors OpenSWATH calibration into a new public CalibrationWorkflow class and implementation, removes the old OpenSwathCalibrationWorkflow and TOPPOpenSwathBase::performCalibration, adds percentile-based window estimation parameters, updates OpenSwathWorkflow/TOPP to delegate to CalibrationWorkflow, and adds tests and build entries. Changes
Sequence Diagram(s)sequenceDiagram
participant TOPP as TOPP/OpenSwathWorkflow
participant Cal as CalibrationWorkflow
participant FS as FileHandler
participant Sw as SwathMaps/SpectrumAccess
participant Trans as TransformationStorage
TOPP->>Sw: prepare swath_maps / load MS1 (optional)
TOPP->>Cal: determineIrtStrategy(targeted_exp, num_runs)
Cal-->>TOPP: IrtStrategy
TOPP->>Cal: prepareIrtExperiments(strategy, targeted_exp, priority_peptides, run_idx, cached_irts)
Cal->>FS: loadIrtExperimentFromFile_(file_path)
FS-->>Cal: irt_experiment
TOPP->>Cal: performCalibration(swath_maps, targeted_exp, cp, cp_ms1, irt_experiments, ...)
Cal->>Sw: extract chromatograms / load MS1 (if needed)
Cal->>Trans: compute RT/mz/IM transformations & estimate windows
Cal-->>TOPP: CalibrationResult (trafo, windows)
TOPP->>FS: optionally write trafo/mzML
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openms/source/ANALYSIS/OPENSWATH/OpenSwathWorkflow.cpp (1)
31-37:⚠️ Potential issue | 🟡 MinorReturn the first MS1 map as documented.
The loop currently keeps the last MS1 map; break on the first match to align with the helper’s intent.🐛 Suggested fix
- for (SignedSize i = 0; i < boost::numeric_cast<SignedSize>(swath_maps.size()); ++i) + for (SignedSize i = 0; i < boost::numeric_cast<SignedSize>(swath_maps.size()); ++i) { // if (swath_maps[i].ms1 && use_ms1_traces_) if (swath_maps[i].ms1) { ms1_map = swath_maps[i].sptr; + break; } }src/topp/OpenSwathWorkflow.cpp (1)
891-1062:⚠️ Potential issue | 🟠 MajorSAMPLE_ONCE iRT sampling is re-run every run.
prepareIrtExperimentsis always called withcached_irts=nullptr, so runs can get different sampled iRTs (especially whenauto_irt:irt_seedis 0), which defeats SAMPLE_ONCE. Cache the first result and reuse it for subsequent runs.🐛 Suggested fix
- Size run_index = 0; + Size run_index = 0; + CalibrationWorkflow::IrtExperiments cached_irts; + bool has_cached_irts = false; for (const StringList& current_run_files : run_groups) { @@ - CalibrationWorkflow::IrtExperiments irt_experiments = calibration_wf.prepareIrtExperiments( - strategy, transition_exp, priority_pep_strings, run_index); + const CalibrationWorkflow::IrtExperiments* cached_ptr = + (strategy == IrtStrategy::SAMPLE_ONCE && has_cached_irts) ? &cached_irts : nullptr; + CalibrationWorkflow::IrtExperiments irt_experiments = calibration_wf.prepareIrtExperiments( + strategy, transition_exp, priority_pep_strings, run_index, cached_ptr); + if (strategy == IrtStrategy::SAMPLE_ONCE && !has_cached_irts && irt_experiments.is_prepared) + { + cached_irts = irt_experiments; + has_cached_irts = true; + }
🤖 Fix all issues with AI agents
In `@src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathWorkflow.h`:
- Around line 279-280: The public method declaration loadMS1Map(const
std::vector<OpenSwath::SwathMap>& swath_maps, bool load_into_memory) has only a
terse /// comment; replace it with a full Doxygen block matching header
guidelines (use `@brief`, `@param` swath_maps, `@param` load_into_memory, and `@return`)
similar to the neighbouring method's style so the API is properly documented;
update the comment directly above the OpenSwath::SpectrumAccessPtr
loadMS1Map(...) declaration in OpenSwathWorkflow.h.
In `@src/openms/source/ANALYSIS/OPENSWATH/CalibrationWorkflow.cpp`:
- Around line 239-258: The CalibrationResult currently leaves QC fields at
defaults; populate result.num_irt_peptides_used, result.rt_rsq and
result.coverage_fraction before returning (right after you set
result.estimated_rt_window and before storing transformations/endProgress). Set
them from the internal values produced during
performRTNormalization()/doDataNormalization_ (e.g. assign
result.num_irt_peptides_used = num_irt_peptides_used_; result.rt_rsq = rt_rsq_;
result.coverage_fraction = coverage_fraction_), or if those internal members do
not exist, compute them from the RT fitting data (e.g. count of IRT peptides
used, R² from the RT transformation stored in result.rt_trafo, and fraction of
RT coverage used) — alternatively remove these QC fields from the public
CalibrationResult if you cannot reliably populate them.
- Around line 447-450: Remove the no-op conditional around the
estimateBestPeptides flag: delete the empty "if (estimateBestPeptides) { }"
block (the variable is set via
irt_detection_param.getValue("estimateBestPeptides").toBool()) so there is no
unused empty branch; if any behavior was intended, implement it inside the block
instead of leaving it empty.
In `@src/tests/topp/CMakeLists.txt`:
- Around line 1866-1871: The -Calibration:files:linear_irt_file argument in the
add_test call for "TOPP_OpenSwathWorkflow_27" is currently a space-delimited
string and should be a semicolon-delimited list; modify the add_test invocation
to pass a semicolon-separated list of the .irt.TraML paths (escape semicolons so
CMake keeps them as a single CLI argument, e.g. replace the space-separated
sequence in the -Calibration:files:linear_irt_file value with semicolon
separators escaped for CMake like "\;"), or alternatively construct a CMake list
variable and pass that variable as the single -Calibration:files:linear_irt_file
argument so positional mapping is exercised correctly.
🧹 Nitpick comments (2)
src/tests/class_tests/openms/source/CalibrationWorkflow_test.cpp (1)
27-253: Consider extracting a small test fixture helper to remove duplicated setup.
Both sections build identical targeted experiments and chromatograms; a shared helper would keep the tests easier to maintain.src/openms/include/OpenMS/ANALYSIS/OPENSWATH/CalibrationWorkflow.h (1)
134-165: Use OpenMS::Size for run counts/indices in the public API.
This keeps the interface consistent with OpenMS container-size conventions. Per the coding guidelines, OpenMS primitive types fromOpenMS/CONCEPT/Types.h(e.g.,Size,SignedSize) should be used instead ofsize_tfor STL container sizes and related counts.Suggested update
- IrtStrategy determineIrtStrategy( - const OpenSwath::LightTargetedExperiment& full_transition_exp, - size_t num_runs = 1 - ) const; + IrtStrategy determineIrtStrategy( + const OpenSwath::LightTargetedExperiment& full_transition_exp, + Size num_runs = 1 + ) const; IrtExperiments prepareIrtExperiments( IrtStrategy strategy, const OpenSwath::LightTargetedExperiment& full_transition_exp, const std::vector<String>& priority_peptides, - size_t run_index = 0, + Size run_index = 0, const IrtExperiments* cached_irts = nullptr );Also update the corresponding implementation in CalibrationWorkflow.cpp (lines 793 and 859).
…rategy selection and handling of NULL_TRANSFORMATION
…alibrationWorkflow class
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/openms/include/OpenMS/ANALYSIS/OPENSWATH/CalibrationWorkflow.h`:
- Around line 308-312: The four public getter methods getEstimatedMzWindow,
getEstimatedImWindow, getEstimatedMs1MzWindow, and getEstimatedMs1ImWindow lack
Doxygen `@brief` documentation; add Doxygen blocks above each method in
CalibrationWorkflow.h using the required style: a single-line `@brief` describing
what the getter returns, a blank line, then any additional details (if needed)
and since there are no parameters or return tags required by your guideline you
may omit `@param` but ensure the brief clearly states the returned value (e.g.,
"Estimated m/z extraction window (populated after calibration)"); keep blocks
consistent across all four getters.
- Around line 67-69: Remove the extra blank line(s) immediately following the
access specifiers in the CalibrationWorkflow.h header: delete the empty line
after "public:" (above the "/// Results from calibration workflow" comment) and
likewise remove the blank line(s) after the matching "private:" block referenced
around lines 314-316 so access specifiers directly precede their first member
declarations; update within the class CalibrationWorkflow to keep no blank line
between the access label and the first comment/member.
In `@src/openms/source/ANALYSIS/OPENSWATH/CalibrationWorkflow.cpp`:
- Around line 279-331: The linear calibration path disables m/z correction but
still leaves IM correction enabled, causing SwathMapMassCorrection::correctIM to
run twice (once in performLinearCalibration_ and again in doDataNormalization_)
and distorting PASEF drift times; fix by disabling IM correction in the linear
step by updating linear_calibration_param (the Param passed into
performLinearCalibration_) to disable the IM/mobility correction flag used by
SwathMapMassCorrection (e.g., set the appropriate key such as "im_correction" or
the project’s mobility correction param to "none"/false) before calling
performLinearCalibration_, or alternatively disable that same IM/mobility
correction in calibration_params_configured passed to doDataNormalization_ if
you prefer IM correction only in the linear step.
🧹 Nitpick comments (2)
src/openms/include/OpenMS/ANALYSIS/OPENSWATH/CalibrationWorkflow.h (2)
126-157: UseOpenMS::Sizefor run counters in the public API.This keeps the new API consistent with OpenMS size conventions and avoids mixing standard types with OpenMS types.
🔧 Suggested change
- IrtStrategy determineIrtStrategy( - const OpenSwath::LightTargetedExperiment& full_transition_exp, - size_t num_runs = 1 - ) const; + IrtStrategy determineIrtStrategy( + const OpenSwath::LightTargetedExperiment& full_transition_exp, + Size num_runs = 1 + ) const; @@ - IrtExperiments prepareIrtExperiments( - IrtStrategy strategy, - const OpenSwath::LightTargetedExperiment& full_transition_exp, - const std::vector<String>& priority_peptides, - size_t run_index = 0, - const IrtExperiments* cached_irts = nullptr - ); + IrtExperiments prepareIrtExperiments( + IrtStrategy strategy, + const OpenSwath::LightTargetedExperiment& full_transition_exp, + const std::vector<String>& priority_peptides, + Size run_index = 0, + const IrtExperiments* cached_irts = nullptr + );Please update the corresponding
.cppdefinitions for signature parity. As per coding guidelines: Use OpenMS primitive types from OpenMS/CONCEPT/Types.h (e.g., Size, SignedSize) instead of standard types like int, unsigned int for STL container sizes.
218-220: Public helper name should not use a trailing underscore.Trailing underscores are reserved for member variables; for a public API this reads like an internal method. Consider renaming (e.g.,
loadIrtExperimentFromFile) or moving it to the private section if it is internal-only. As per coding guidelines: Use lowerCamelCase for method names; use snake_case for variable names; use trailing underscore for private and protected member variables.
…sors in CalibrationWorkflow
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/openms/include/OpenMS/ANALYSIS/OPENSWATH/CalibrationWorkflow.h`:
- Around line 65-105: Add Doxygen `@brief` blocks for the public structs and
constructor/destructor: document the CalibrationResult struct (briefly
describing it holds RT/IM transformations and estimated extraction windows), the
IrtExperiments struct (briefly describing linear/nonlinear experiments, strategy
and readiness flag), the default constructor CalibrationWorkflow() and the
destructor ~CalibrationWorkflow(); follow project style by using an `@brief` line,
a blank line, then longer details as needed and include `@param` tags only if the
ctor/dtor have parameters (they do not), ensuring the comments sit immediately
above the declarations for CalibrationResult, IrtExperiments,
CalibrationWorkflow(), and ~CalibrationWorkflow().
- Around line 219-303: The Doxygen param tags are incorrect: update the comment
blocks for performRTNormalization and doDataNormalization_ so that swath_maps is
marked `@param`[in,out] (because the function mutates the provided swath_maps) and
irt_mzml_out is marked `@param`[in] (it's an input path), ensure the `@brief` is
followed by a blank line before the detailed description and that all parameters
use `@param`[in], `@param`[out] or `@param`[in,out] as appropriate (refer to
performRTNormalization, doDataNormalization_, swath_maps, and irt_mzml_out to
locate the exact blocks to edit).
Contributor
|
Nice |
5 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
iRT calibration logic in OpenSwathWorkflow is split across
src/topp/OpenSwthWorkflow,src/openms/source/ANALYSIS/OPENSWATH/OpenSwathWorkflowandsrc/openms/include/OpenMS/APPLICATIONS/OpenSwathBase. This PR refactors the main calibration logic into a new separate reusableCalibrationWorkflowclass.Refactoring and Codebase Simplification:
OpenSwathCalibrationWorkflowclass and all its methods fromOpenSwathWorkflow.h, centralizing calibration logic elsewhere and reducing code duplication.performCalibrationmethod fromOpenSwathBase.h, reflecting the refactored calibration workflow.New Functionality and API Changes:
loadMS1MaptoOpenSwathWorkflow, which loads the first MS1 map from a set of swath maps, with an option to load data into memory. [1] [2] [3]mz_estimation_percentile_andim_estimation_percentile_for finer control over mass and ion mobility window estimation inSwathMapMassCorrection.h.Project Structure and Build System:
CalibrationWorkflow.hto the CMake sources list, ensuring the new calibration workflow header is included in the build.OpenSwathWorkflow.cppto reference the newCalibrationWorkflow.h.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
Refactor
Tests