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

A tiny change: use a phylogenetic directory #15

Merged
merged 16 commits into from
Jan 9, 2024
Merged

Conversation

j23414
Copy link
Contributor

@j23414 j23414 commented Nov 29, 2023

Description of proposed changes

A tiny change ;) Use a phylogenetic directory to follow the pathogen-repo-template.

Related issue(s)

Checklist

  • Checks pass
  • Manual check passes
git clone https://github.com/nextstrain/dengue.git
cd dengue
git checkout phylogenetic_dir
cd phylogenetic
nextstrain build . 

@j23414 j23414 requested a review from a team November 29, 2023 23:28
@j23414 j23414 marked this pull request as draft November 29, 2023 23:30
Move phylogenetic workflow from top-level to folder phylogenetic in order
to follow the Pathogen Repo Template:

https://github.com/nextstrain/pathogen-repo-template
@j23414 j23414 marked this pull request as ready for review December 5, 2023 18:22
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.

The majority of the changes look good to me 👍

My only request to is update the CI workflow since it currently does not fully work as intended. Looking at the latest CI job on this branch, there are no artifacts uploaded due to the bug so we cannot inspect the build outputs.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
joverlee521 added a commit to nextstrain/pathogen-repo-guide that referenced this pull request Jan 4, 2024
Reduce confusion of the use of the profiles' config files. They are to
be seen as additional profiles' config on top of the default configs.
Stems from discussion with @j23414 for review of
nextstrain/dengue#15.

Note we not using `config.yaml` because this is used by the Snakemake
profiles configuration.¹

¹ https://snakemake.readthedocs.io/en/stable/executing/cli.html#profiles
@j23414
Copy link
Contributor Author

j23414 commented Jan 8, 2024

Hi @joverlee521! This PR is ready for review. Following our earlier discussion, I've implemented further changes, but am open to further revisions.

  • Use decompressed example files
  • Refactor and relocate certain dengue-specific functions from the phylogenetic/Snakefile to a more generalized dengue_config.yaml

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.

Thanks for working through this! I caught a couple smaller things but should be good to merge!

[auspice]: https://docs.nextstrain.org/projects/auspice/en/stable/index.html
[Installing Nextstrain guide]: https://docs.nextstrain.org/en/latest/install.html
[Running a Pathogen Workflow guide]: https://docs.nextstrain.org/en/latest/tutorials/running-a-workflow.html
- [Contributor documentation](./CONTRIBUTING.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realizing this CONTRIBUTING.md file also does not exist in this repo.

Comment on lines 54 to 59
Alternatively, you can run the build using the
example data provided in this repository. Before running the build, copy the
example sequences into the `data/` directory like so:

mkdir -p data/
cp example_data/dengue* data/
Copy link
Contributor

Choose a reason for hiding this comment

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

One other benefit to having the CI profile is users no longer have to manually copy over the example data.
These instructions can be updated to use

nextstrain build .  --configfile profiles/ci/profiles_config.yaml

@j23414 j23414 merged commit a313f45 into main Jan 9, 2024
8 checks passed
@j23414 j23414 deleted the phylogenetic_dir branch January 9, 2024 21:37
joverlee521 added a commit to nextstrain/pathogen-repo-guide that referenced this pull request Jan 17, 2024
Reduce confusion of the use of the customizations' config files. They
are to be seen as additional custom configs on top of the
default configs.

Stems from discussion with @j23414 for review of nextstrain/dengue#15.
joverlee521 added a commit to nextstrain/pathogen-repo-guide that referenced this pull request Jan 19, 2024
Reduce confusion of the use of the customizations' config files. They
are to be seen as additional custom configs on top of the
default configs.

Stems from discussion with @j23414 for review of nextstrain/dengue#15.
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.

2 participants