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

Adding assembly_mean_coverage metrics for flu in TheiaCoV_Illumina_PE_PHB #314

Merged
merged 16 commits into from
Jan 24, 2024

Conversation

jrotieno
Copy link
Contributor

@jrotieno jrotieno commented Jan 23, 2024

This PR closes #313.

🗑️ This dev branch should be deleted after merging to main.

🧠 Aim, Context and Functionality

This PR adds the assembly_mean_coverage metrics for influenza HA and NA in the TheiaCoV_Illumina_PE_PHB workflow which was previously only available for other pathogens.

🛠️ Impacted Workflows/Tasks & Changes Being Made

This will affect the behavior of the workflow(s) even if users don’t change any workflow inputs relative to the last version : Yes (An output in assembly_mean_coverage where organism is "flu" that was previously empty)

Running this workflow on different occasions could result in different results, e.g. due to use of a live database, "latest" docker image, or stochastic data processing : Yes (Though not as a result of this specific PR)

📋 Workflow/Task Step Changes

🔄 Data Processing

Docker/software or software versions changed: N/A

Databases or database versions changed: N/A

Data processing/commands changed:
The workflow now calls the assembly metric task when sample HA and/or NA bam files from IRMA are available.

File processing changed:
IRMA task now also outputs HA and NA bam files.

Compute resources changed: N/A

➡️ Inputs

Not changed

⬅️ Outputs

Now when organism is "flu", instead of the workflow returning an empty output for assembly_mean_coverage, there will be a value HA:<coverage>, NA:<coverage>

🧪 Testing

Test Dataset

A set of four samples were used:

  1. Influenza B Yamagata
  2. Influenza A H3N2
  3. Influenza A H1N1
  4. Influenza A H1N1 with only NA subtyped, i.e. expecting HA to fail

Commandline Testing with MiniWDL or Cromwell (optional)

Local testing done for the IRMA task and works as expected.

Terra Testing

Testing done here: https://app.terra.bio/#workspaces/cdph-terrabio-taborda-manual/Global_tree_testing/job_history/7189276f-f2b7-48be-b64d-3426353d087e

Suggested Scenarios for Reviewer to Test

Subtypes other than those listed here.

Theiagen Version Release Testing (optional)

🔬 Final Developer Checklist

  • The workflow/task has been tested locally and results, including file contents, are as anticipated
  • The workflow/task has been tested on Terra and results, including file contents, are as anticipated
  • The CI/CD has been adjusted and tests are passing (to be completed by Theiagen developer)
  • Code changes follow the style guide

🎯 Reviewer Checklist

  • All impacted workflows/tasks have been tested on Terra with a different dataset than used for development
  • All reviewer-suggested scenarios have been tested and any additional
  • All changed results have been confirmed to be accurate
  • All workflows/tasks impacted by change/s have been tested using a standard validation dataset to ensure no unintended change of functionality
  • All code adheres to the style guide
  • MD5 sums have been updated
  • The PR author has addressed all comments

🗂️ Associated Documentation (to be completed by Theiagen developer)

  • Relevant documentation on the Public Health Resources "PHB Main" has been updated
  • Workflow diagrams have been updated to reflect changes

@jrotieno jrotieno marked this pull request as ready for review January 23, 2024 10:28
@cimendes cimendes marked this pull request as draft January 24, 2024 10:56
@jrotieno jrotieno marked this pull request as ready for review January 24, 2024 12:45
@sage-wright sage-wright marked this pull request as draft January 24, 2024 14:45
Copy link

@emily-smith1 emily-smith1 left a comment

Choose a reason for hiding this comment

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

Tested successfully on 38 influenza samples. Output in the assembly_mean_coverage column on Terra appears as expected with values for HA and NA.

@jrotieno jrotieno marked this pull request as ready for review January 24, 2024 19:04
@jrotieno jrotieno merged commit 2f1cb6b into main Jan 24, 2024
4 of 6 checks passed
@jrotieno jrotieno deleted the jro-flu-assembly-mean-coverage branch January 26, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing assembly_mean_coverage value for flu
2 participants