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

Update/arborator gas 0.1.3 #27

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Update/arborator gas 0.1.3 #27

merged 5 commits into from
Jan 15, 2025

Conversation

apetkau
Copy link
Member

@apetkau apetkau commented Jan 15, 2025

Updates the container for arborator in order to update the genomic_address_service dependency to version 0.1.3 to fix an issue with branch lengths in newick files.

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!
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • CHANGELOG.md is updated.

@apetkau apetkau marked this pull request as draft January 15, 2025 15:55
Copy link

github-actions bot commented Jan 15, 2025

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 257826e

+| ✅ 143 tests passed       |+
#| ❔  28 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

  • files_exist - File is ignored: assets/nf-core-arboratornf_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-arboratornf_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-arboratornf_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
  • nextflow_config - Config variable ignored: params.max_cpus
  • 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-arboratornf_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-arboratornf_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-arboratornf_logo_dark.png
  • files_unchanged - File ignored due to lint config: docs/README.md
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/arboratornf/arboratornf/.github/workflows/awstest.yml
  • actions_awsfulltest - actions_awsfulltest
  • pipeline_name_conventions - pipeline_name_conventions

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.1
  • Run at 2025-01-15 21:31:51

// The goal of this test is to make sure that the produced tree has correct branches
// So other comparisons for metadata, etc, were removed
def actual_arborator_tree_1 = path("$launchDir/results/arborator/1_tree.nwk")
def expected_arborator_tree_1 = path("$baseDir/tests/data/arborator/1/tree_all_single_partition.nwk")
Copy link
Member Author

Choose a reason for hiding this comment

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

I included this test which focuses on verifying the produced newick file when all genomes are in the same partition is valid (e.g., has correct branch lengths after updating GAS to 0.1.3). I removed all other checks (e.g., related to metadata/summaries) from this test case.

For additional information, see the screenshot below. The left tree is the expected_arborator_tree_1, with correct branch lengths (all leaves line up on the right side). The right tree is the older, incorrect tree when using the older arborator container with an older version of the GAS dependency (you can see leaves do not all line up on the right side).

image

For more context on this issue, see phac-nml/genomic_address_service#15

@apetkau apetkau marked this pull request as ready for review January 15, 2025 18:55
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 Aaron. Just a couple small comments.

CHANGELOG.md Outdated
@@ -3,6 +3,10 @@
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.3.2] - 2025-01-15

- Updated the build of the arborator container in order to update the `genomic_address_service` dependency to [version 0.1.3](https://github.com/phac-nml/genomic_address_service/releases/tag/0.1.3), which fixes an issue with branch lengths in newick files.
Copy link
Member

Choose a reason for hiding this comment

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

Newick should be capitalized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks. Fixed 257826e

Comment on lines +121 to +122
tag "pipeline"
tag "pipeline_all"
Copy link
Member

Choose a reason for hiding this comment

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

Is the double tag here intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I wanted to have a separate tag so I could run this test alone.

@apetkau apetkau merged commit e963f03 into dev Jan 15, 2025
4 checks passed
@apetkau apetkau deleted the update/arborator-gas-0.1.3 branch January 15, 2025 22:05
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