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

Harmonize ingest with pathogen repo guide #35

Merged
merged 10 commits into from
Mar 19, 2024

Conversation

j23414
Copy link
Contributor

@j23414 j23414 commented Mar 18, 2024

Description of proposed changes

Harmonize the ingest workflow to match the Pathogen Repo Guide. This may simplify reviewing the E_gene PR and Nextclade rules PR.

Related issue(s)

Checklist

  • Checks pass

@j23414 j23414 linked an issue Mar 18, 2024 that may be closed by this pull request
7 tasks
@j23414 j23414 requested a review from a team March 18, 2024 10:22
@j23414 j23414 changed the title 22 harmonize with pathogen repo guide Harmonize ingest with pathogen repo guide Mar 18, 2024
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Changes look good to me!

I left comments on some additional clean up that I missed in nextstrain/zika@cee62cf and was also missed in #34

ingest/Snakefile Outdated

configfile: "config/config.yaml"

configfile: "defaults/config.yaml"

send_slack_notifications = config.get("send_slack_notifications", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed this when I was reviewing nextstrain/zika@cee62cf, but this line can be removed since we are no longer sending Slack notifications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Dropped slack related code and docs in 2e14f00

ingest/Snakefile Outdated
@@ -57,12 +54,13 @@ rule all:
_get_all_targets,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto missed this when I was reviewing nextstrain/zika@cee62cf, but I think the all rule can be simplified to the default targets and the entire _get_all_targets function can be removed.

rule all:
    input:
        expand(["results/sequences_{serotype}.fasta", "results/metadata_{serotype}.tsv"], serotype=serotypes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I like the simplified default targets. Fixed in ed1bf1f

@j23414 j23414 force-pushed the 22-harmonize-with-pathogen-repo-guide branch from ed1bf1f to 2ffef24 Compare March 19, 2024 21:01
@j23414 j23414 merged commit abb8984 into main Mar 19, 2024
8 checks passed
@j23414 j23414 deleted the 22-harmonize-with-pathogen-repo-guide branch March 19, 2024 21:02
@j23414 j23414 mentioned this pull request May 23, 2024
4 tasks
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.

Harmonize with pathogen repo guide
2 participants