Skip to content
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.

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

feat: add msi analysis for TO workflow #1463

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

khurrammaqbool
Copy link
Contributor

@khurrammaqbool khurrammaqbool commented Jul 5, 2024

Description

Add MSI analysis for tumor-only workflow.

[PR description]

Added

  • MSI analysis for tumor-only workflow

Documentation

  • N/A
  • Updated Balsamic documentation to reflect the changes as needed for this PR.
    • [Document Name]

Tests

Feature Tests

  • N/A
  • Test [Description]
    • MSI WGS-TO:
Total_Number_of_Sites Number_of_Somatic_Sites MSI
639289 7480 1.17

Pipeline Integrity Tests

  • Report deliver (generation of the .hk file)
    • N/A
    • Verified
  • TGA T/O Workflow
    • N/A
    • Verified
  • TGA T/N Workflow
    • N/A
    • Verified
  • UMI T/O Workflow
    • N/A
    • Verified
  • UMI T/N Workflow
    • N/A
    • Verified
  • WGS T/O Workflow
    • N/A
    • Verified
  • WGS T/N Workflow
    • N/A
    • Verified
  • QC Workflow
    • N/A
    • Verified
  • PON Workflow
    • N/A
    • Verified

Clinical Genomics Stockholm

Documentation

  • Atlas documentation
    • N/A
    • Updated: [Link]
  • Web portal for Clinical Genomics
    • N/A
    • Updated: [Link]

Panel of Normal specific criteria

User Changes

  • N/A
  • This PR affects the output files or results.
    • User feedback is considered unnecessary because [Justification].
    • Affected users have been included in the development process and given a chance to provide feedback.

Infrastructure Changes

  • Stored files in Housekeeper
    • N/A
    • Updated: [Link]
  • CG (CLI and delivered/uploaded files)
    • N/A
    • Updated: [Link]
  • Servers (configuration files on Hasta)
    • N/A
    • Updated: [Link]
  • Scout interface
    • N/A
    • Updated: [Link]

Checklist

Important

Ensure that all checkboxes below are ticked before merging.

For Developers

  • PR Description
    • Provided a comprehensive description of the PR.
    • Linked relevant user stories or issues to the PR.
  • Documentation
    • Verified and updated documentation if necessary.
  • Tests
    • Described and tested the functionality addressed in the PR.
    • Ensured integration of the new code with existing workflows.
    • Confirmed that meaningful unit tests were added for the changes introduced.
    • Checked that the PR has successfully passed all relevant code smells and coverage checks.
  • Review
    • Addressed and resolved all the feedback provided during the code review process.
    • Obtained final approval from designated reviewers.

For Reviewers

  • Code
    • Code implements the intended features or fixes the reported issue.
    • Code follows the project's coding standards and style guide.
  • Documentation
    • Pipeline changes are well-documented in the CHANGELOG and relevant documentation.
  • Tests
    • The author provided a description of their manual testing, including consideration of edge cases and boundary
      conditions where applicable, with satisfactory results.
  • Review
    • Confirmed that the developer has addressed all the comments during the code review.

@khurrammaqbool khurrammaqbool requested a review from a team as a code owner July 5, 2024 10:30
@khurrammaqbool khurrammaqbool self-assigned this Jul 5, 2024
@khurrammaqbool khurrammaqbool added the Feature New feature label Jul 5, 2024
@khurrammaqbool khurrammaqbool added this to the Release 16 milestone Jul 5, 2024
@khurrammaqbool khurrammaqbool linked an issue Jul 5, 2024 that may be closed by this pull request
3 tasks
Copy link
Contributor

@ivadym ivadym left a comment

Choose a reason for hiding this comment

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

Looks good 👍

tests/conftest.py Outdated Show resolved Hide resolved
BALSAMIC/commands/options.py Show resolved Hide resolved
BALSAMIC/constants/cluster_analysis.json Outdated Show resolved Hide resolved
BALSAMIC/snakemake_rules/annotation/msi_tumor_only.rule Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Jul 5, 2024

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.49%. Comparing base (7d529e6) to head (e08da08).
Report is 13 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1463   +/-   ##
========================================
  Coverage    99.48%   99.49%           
========================================
  Files           40       40           
  Lines         1932     1963   +31     
========================================
+ Hits          1922     1953   +31     
  Misses          10       10           
Flag Coverage Δ
unittests 99.49% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mathiasbio
Copy link
Contributor

I forget! We talked a bit about adding validation samples for this feature, like some sample with known high and low MSI which we can use to verify that the feature is working, and continues to keep working for future releases. I don't remember what you said about it, but did we have any such sample to add?

I understand that actually verifying it for the tumor only validation cases is impossible without the PON, but it would be nice to make sure that we at least have a way to validate it in the validation of release 16 after the PON has been added.

And for the WGS workflow where a PON exists, do we have a validation sample we could test?

@mathiasbio
Copy link
Contributor

mathiasbio commented Jul 8, 2024

Also I'd love it if you could:

  • add a bit more in the description, like what the difference is between the tumor normal and the tumor only (that a PON is required)
  • add some documentation in Atlas and in balsamic docs about the new MSI PON (same area maybe where we added for CNVkit and for GENS)
  • that testing the TGA tumor only workflow for MSI is blocked by the lack of a PON, but maybe give some justification for why this testing shouldn't be necessary (maybe because it basically shares the exact same code as the WGS workflow? I don't know).
  • and maybe finally, check the boxes for which workflows you have tested, just so we know that the analyses have finished successfully

@khurrammaqbool
Copy link
Contributor Author

I forget! We talked a bit about adding validation samples for this feature, like some sample with known high and low MSI which we can use to verify that the feature is working, and continues to keep working for future releases. I don't remember what you said about it, but did we have any such sample to add?

I understand that actually verifying it for the tumor only validation cases is impossible without the PON, but it would be nice to make sure that we at least have a way to validate it in the validation of release 16 after the PON has been added.

And for the WGS workflow where a PON exists, do we have a validation sample we could test?

I added results from MSI analysis using MSI-PON for WGS-TO validation case.

@jemten jemten modified the milestones: Release 16, Release 17 Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

[User Story] Add MSI analysis
4 participants