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.

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

feat: add msisensorpro TN #1454

Merged
merged 10 commits into from
Jun 26, 2024
Merged

feat: add msisensorpro TN #1454

merged 10 commits into from
Jun 26, 2024

Conversation

khurrammaqbool
Copy link
Collaborator

@khurrammaqbool khurrammaqbool commented Jun 24, 2024 •
edited
Loading

Description

Add msisensor-pro for tumor-normal workflows.

[PR description]

Added

  • msisensor-pro

Documentation

  • N/A
  • Updated Balsamic documentation to reflect the changes as needed for this PR.
    • docs/balsamic_methods.rst
    • docs/balsamic_annotation.rst

Tests

Feature Tests

  • N/A
  • Test [Description]
    • [Screenshot]

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 self-assigned this Jun 24, 2024
@khurrammaqbool khurrammaqbool requested a review from a team as a code owner June 24, 2024 07:33
@khurrammaqbool khurrammaqbool linked an issue Jun 24, 2024 that may be closed by this pull request
[User Story] Add MSI analysis #1443
Open
3 tasks
@khurrammaqbool khurrammaqbool changed the title feat: add msisensorpro feat: add msisensorpro TN Jun 24, 2024
@khurrammaqbool khurrammaqbool added this to the Release 16 milestone Jun 24, 2024
@codecov Codecov
Copy link

codecov bot commented Jun 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.48%. Comparing base (7d529e6) to head (bc5a58e).
Report is 9 commits behind head on develop.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #1454   +/-   ##
========================================
  Coverage    99.48%   99.48%           
========================================
  Files           40       40           
  Lines         1932     1944   +12     
========================================
+ Hits          1922     1934   +12     
  Misses          10       10
Flag Coverage Δ
unittests 99.48% <ø> (+<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.

@khurrammaqbool
Copy link
Collaborator Author

WGS TN

Total_Number_of_Sites Number_of_Somatic_Sites %
988874 315 0.03

TGA TN

Total_Number_of_Sites Number_of_Somatic_Sites %
18362 9486 51.66

@mathiasbio
Copy link
Collaborator

Can we find a way to add some more information about what is being developed, either here or in the user story? How are the results validated, and how will they be presented to the clinicians, is there an active PR in Scout and CG for instance to include more files in the scout.yaml for upload? Maybe if this PR is just a small part of the whole feature this general info could be added in the user-story instead: #1443 :D Maybe it's just me but I would love to know a bit more about the plan : )

@khurrammaqbool
Copy link
Collaborator Author

Can we find a way to add some more information about what is being developed, either here or in the user story? How are the results validated, and how will they be presented to the clinicians, is there an active PR in Scout and CG for instance to include more files in the scout.yaml for upload? Maybe if this PR is just a small part of the whole feature this general info could be added in the user-story instead: #1443 :D Maybe it's just me but I would love to know a bit more about the plan : )

I started looking the final results and added the output above. These are suppose to be included in the PDF report and it seems I forgot to convert the file to pdf before merge, I will do so now :-)

ivadym
ivadym reviewed Jun 25, 2024
View reviewed changes
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.

Nice work! 👍

I would love to see a documented user story outlining how and what should be delivered to the customer, as Mathias mentioned. That could be very useful before diving into a PR review

CHANGELOG.rst Outdated Show resolved Hide resolved
BALSAMIC/snakemake_rules/annotation/somatic_computations.rule Outdated Show resolved Hide resolved
BALSAMIC/snakemake_rules/annotation/somatic_computations.rule Show resolved Hide resolved
BALSAMIC/snakemake_rules/annotation/somatic_computations.rule Outdated Show resolved Hide resolved
BALSAMIC/snakemake_rules/annotation/somatic_computations.rule Outdated Show resolved Hide resolved
BALSAMIC/snakemake_rules/annotation/somatic_computations.rule Outdated Show resolved Hide resolved
BALSAMIC/snakemake_rules/report/generate_pdf.rule Outdated Show resolved Hide resolved
BALSAMIC/snakemake_rules/report/generate_pdf.rule Show resolved Hide resolved
@khurrammaqbool
Copy link
Collaborator Author

khurrammaqbool commented Jun 26, 2024 •
edited
Loading

Nice work! 👍

I would love to see a documented user story outlining how and what should be delivered to the customer, as Mathias mentioned. That could be very useful before diving into a PR review

I updated the docs.

ivadym
ivadym approved these changes Jun 26, 2024
View reviewed changes
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 💯

Could you share the CNV report to check how that PDF page looks like and if it aligns with the rest of the tables format?

docs/balsamic_annotation.rst Outdated Show resolved Hide resolved
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@khurrammaqbool
Copy link
Collaborator Author

Looks good 💯

Could you share the CNV report to check how that PDF page looks like and if it aligns with the rest of the tables format?

image

@khurrammaqbool khurrammaqbool merged commit 6f48313 into develop Jun 26, 2024
8 checks passed
@khurrammaqbool khurrammaqbool deleted the feat/add_msi_analysis_tn branch June 26, 2024 15:14
@mathiasbio
Copy link
Collaborator

A bit late to the party here! But is that the entire section of the MSI results in the report? There's no header to the table at all? I think it would be great if we could at least have a header so that the clinicians know what they're looking at :D (if there is none)

@ivadym
Copy link
Contributor

ivadym commented Jun 27, 2024

A bit late to the party here! But is that the entire section of the MSI results in the report? There's no header to the table at all? I think it would be great if we could at least have a header so that the clinicians know what they're looking at :D (if there is none)

I agree! Could you also share the actual PDF of the CNV report as well so we can see the resolution?

@mathiasbio mathiasbio mentioned this pull request Oct 17, 2024
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathiasbio mathiasbio mathiasbio left review comments

@ivadym ivadym ivadym approved these changes

Assignees

@khurrammaqbool khurrammaqbool

Labels
None yet
Projects
BALSAMIC
Status: Completed
Milestone
Release 16
Development

Successfully merging this pull request may close these issues.

[User Story] Add MSI analysis
3 participants
@khurrammaqbool @mathiasbio @ivadym