8000 add isobaric correction defaults for tmt6plex and 11plex by timosachsenberg · Pull Request #7601 · OpenMS/OpenMS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add isobaric correction defaults for tmt6plex and 11plex #7601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Oct 7, 2024
Merged

add isobaric correction defaults for tmt6plex and 11plex #7601

merged 8 commits into from
Oct 7, 2024

Conversation

timosachsenberg
Copy link
Contributor
@timosachsenberg timosachsenberg commented Oct 4, 2024 •
edited
Loading

User description

Description

correction matrix

  • add tmt6plex
  • add tmt11plex

Checklist

  • Make sure that you are listed in the AUTHORS file
  • Add relevant changes and new features to the CHANGELOG file
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes
  • Updated or added python bindings for changed or new classes (Tick if no updates were necessary.)

How can I get additional information on failed tests during CI

Click to expand If your PR is failing you can check out
  • The details of the action statuses at the end of the PR or the "Checks" tab.
  • http://cdash.openms.de/index.php?project=OpenMS and look for your PR. Use the "Show filters" capability on the top right to search for your PR number.
    If 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.
  • setting the label "NoJenkins" will skip tests for this PR on jenkins (saves resources e.g., on edits that do not affect tests)
  • commenting with rebuild jenkins will retrigger Jenkins-based CI builds

⚠️ Note: Once you opened a PR try to 8000 minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).

PR Type

bug_fix

Description

  • Removed the special case handling for "tmt11plex" in the IsobaricIsotopeCorrector where the correction matrix being an identity matrix was previously allowed.
  • Now, an exception is consistently thrown if the correction matrix is an identity matrix, ensuring that a valid isotope correction matrix is provided.

Changes walkthrough 📝

Relevant files
Bug fix
IsobaricIsotopeCorrector.cpp
Remove special case for "tmt11plex" in correction matrix validation

src/openms/source/ANALYSIS/QUANTITATION/IsobaricIsotopeCorrector.cpp

  • Removed special case handling for "tmt11plex" when the correction
    matrix is the identity matrix.
  • Ensured that an exception is thrown for identity matrices, enforcing
    the need for a valid correction matrix.
    		•  +3/-7     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @github-actions GitHub Actions
    Copy link
    Contributor
    github-actions bot commented Oct 4, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Exception Handling
    The new exception thrown when the correction matrix is an identity matrix might not handle all edge cases, such as specific quant methods that might intentionally use an identity matrix. This change could potentially break existing workflows that depend on the previous behavior.

    @github-actions GitHub Actions
    Copy link
    Contributor
    github-actions bot commented Oct 4, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Reintroduce special case handling for "tmt11plex" to avoid potential issues with identity matrix corrections

    Consider reintroducing the special case handling for "tmt11plex" where the
    correction matrix is the identity matrix. This condition was removed in the PR,
    which might lead to unintended behavior if "tmt11plex" is still a valid method that
    should be treated differently.

    src/openms/source/ANALYSIS/QUANTITATION/IsobaricIsotopeCorrector.cpp [47-49]

    -throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION,
-                                  "IsobaricIsotopeCorrector: The given isotope correction matrix is an identity matrix leading to no correction. "
-                                  "Please provide a valid isotope_correction matrix as it was provided with the sample kit!");
+if (quant_method->getMethodName() != "tmt11plex")
+{        
+  throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION,
+                                    "IsobaricIsotopeCorrector: The given isotope correction matrix is an identity matrix leading to no correction. "
+                                    "Please provide a valid isotope_correction matrix as it was provided with the sample kit!");
+}
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a potential oversight in the PR by reintroducing a previously existing special case for "tmt11plex". This is crucial if "tmt11plex" still requires different handling when the correction matrix is an identity matrix, preventing unintended exceptions and ensuring correct functionality.

    		9

    @timosachsenberg timosachsenberg changed the title Update IsobaricIsotopeCorrector.cpp add isobaric correction defaults for tmt6plex and 11plex Oct 4, 2024
    @timosachsenberg timosachsenberg mentioned this pull request Oct 4, 2024
    Merged
    11 tasks
    timosachsenberg
    timosachsenberg commented Oct 4, 2024
    View reviewed changes
    src/openms/source/ANALYSIS/QUANTITATION/IsobaricIsotopeCorrector.cpp
    {
    throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION,

    throw Exception::InvalidParameter(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION,
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    special treatment of tmt11plex (and 6plex) not needed anymore

    @timosachsenberg timosachsenberg requested a review from poshul October 4, 2024 15:12
    @timosachsenberg
    Copy link
    Contributor Author

    this is a hot fix that should go into 3.2.1

    @timosachsenberg timosachsenberg enabled auto-merge (squash) October 7, 2024 09:43
    @timosachsenberg timosachsenberg merged commit b4034dc into develop Oct 7, 2024
    13 checks passed
    @timosachsenberg timosachsenberg deleted the timosachsenberg-patch-4 branch October 7, 2024 10:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    645E
    Reviewers

    @poshul poshul poshul approved these changes

    @cbielow cbielow Awaiting requested review from cbielow

    Assignees
    No one assigned
    Labels
    bug_fix Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @timosachsenberg @poshul
    0