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

restore vadr_num_alerts string output to theiacov_fasta workflow #307

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

kapsakcj
Copy link
Contributor

@kapsakcj kapsakcj commented Jan 17, 2024

Adding back the vadr_num_alerts string output to theiacov_fasta workflow. It appears that this output was accidentally removed in PR #194 and went unnoticed until we ran the TheiaValidate workflow for the 1.3.0 release.

This PR closes #306

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

🧠 Aim, Context and Functionality

🛠️ 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 : No

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 : No

📋 Workflow/Task Step Changes

🔄 Data Processing

Nothing has changed, we are just re-adding an output column to the workflow

Docker/software or software versions changed: N/A

Databases or database versions changed: N/A

Data processing/commands changed: N/A

File processing changed: N/A

Compute resources changed: N/A

➡️ Inputs

N/A

⬅️ Outputs

  • String? vadr_num_alerts = vadr.num_alerts added to theiacov_fasta WDL wf file

🧪 Testing

Test Dataset

re-tested in Terra using TheiaCov_FASTA validation dataset (includes SC2, Mpox, Flu)

Commandline Testing with MiniWDL or Cromwell (optional)

Testing just done in Terra, no local testing necessary

Terra Testing

https://app.terra.bio/#workspaces/theiagen-validations/PHB_Validation_v1-3-0/job_history/1098810f-408d-42d3-8e20-23ce5530f5eb

Suggested Scenarios for Reviewer to Test

run anything through theiacov_fasta

Theiagen Version Release Testing (optional)

-Will changes require functional or validation testing (checking outputs etc) during the release? Yes, validation testing
-Do new samples need to be added to validation datasets? If so, upload these to the appropriate validation workspace Google bucket (). Please describe the new samples here and why these have been chosen. No
-Are there any output files that should be checked after running the version release testing? No

🔬 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

@sage-wright sage-wright marked this pull request as ready for review January 17, 2024 17:17
Copy link
Member

@sage-wright sage-wright left a comment

Choose a reason for hiding this comment

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

🚀

@sage-wright sage-wright merged commit c3f3b70 into main Jan 17, 2024
5 checks passed
@sage-wright sage-wright deleted the cjk-theiacov-fasta-vadralerts branch January 17, 2024 17:18
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.

vadr_num_alerts not output in TheiaCoV_FASTA
2 participants