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

Updated to support metadata and added ArborView HTML app functionality. #22

Merged
merged 14 commits into from
Jul 9, 2024

Conversation

kylacochrane
Copy link
Contributor

@kylacochrane kylacochrane commented Jul 4, 2024

This PR generates a TSV-formatted metadata file from the input samplesheet and merges it with the PHYML phylogenetic tree. The combined data is then processed through the ArborView HTML app to produce dendrograms with contextual metadata.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

Copy link

github-actions bot commented Jul 4, 2024

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 32cb79a

+| ✅ 143 tests passed       |+
#| ❔  27 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 2.1.0
  • schema_lint - Schema $id should be https://raw.githubusercontent.com/phac-nml/snvphylnfc/master/nextflow_schema.json
    Found https://raw.githubusercontent.com/phac-nml/snvphylnfc/main/nextflow_schema.json
  • nfcore_yml - nf-core version not set in .nf-core.yml

❔ Tests ignored:

  • files_exist - File is ignored: assets/nf-core-snvphylnfc_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-snvphylnfc_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-snvphylnfc_logo_dark.png
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: lib/Utils.groovy
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/WorkflowSnvphylnfc.groovy
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: .github/workflows/branch.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: assets/nf-core-snvphylnfc_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-snvphylnfc_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-snvphylnfc_logo_dark.png
  • files_unchanged - File ignored due to lint config: docs/README.md
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/snvphylnfc/snvphylnfc/.github/workflows/awstest.yml
  • actions_awsfulltest - actions_awsfulltest
  • pipeline_name_conventions - pipeline_name_conventions

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-07-09 14:06:38

@kylacochrane kylacochrane marked this pull request as ready for review July 5, 2024 19:12
Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

This looks amazing. Thanks so much @kylacochrane 😄

I have a few comments below. In addition:

  1. Could you add a message in the CHANGELOG.md about this change? Could you also reference this PR in that message (like what is done for the mikrokondo changelog: https://github.com/phac-nml/mikrokondo/blob/main/CHANGELOG.md)?
  2. I am wondering if SNVPhyl will work even if all the metadata_1 ... metadata_8 columns are left out of the samplesheet.csv? It would be nice if it did, since this update would be backwards-compatible with the previous samplesheet format. Could you test this out?

assets/samplesheet.csv Outdated Show resolved Hide resolved
modules/local/appendmetadata/main.nf Outdated Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
tests/pipelines/main.nf.test Show resolved Hide resolved
@kylacochrane
Copy link
Contributor Author

I can confirm that the pipeline functions correctly even without metadata columns, ensuring backward compatibility of the input samplesheet. You can find the relevant test here: aa4f291

Additionally, another test was added that includes only three metadata columns, renames those columns, and provides partial metadata: aa4f291

The CHANGELOG.md was updated here: 94c4f3c

Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

Thanks so much for all your great work Kyla 😄 . I have two more small comments.

modules/local/appendmetadata/main.nf Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

Thanks so much Kyla

Copy link
Member

@emarinier emarinier left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks Kyla.

@kylacochrane kylacochrane merged commit 5f57e92 into dev Jul 9, 2024
4 checks passed
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.

4 participants