Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Dev -> Master for v2.3 release #273

Merged
merged 280 commits into from
Feb 4, 2022
Merged

Dev -> Master for v2.3 release #273

merged 280 commits into from
Feb 4, 2022

Conversation

drpatelh
Copy link
Member

@drpatelh drpatelh commented Feb 3, 2022

No description provided.

antunderwood and others added 30 commits January 12, 2022 13:39
Correctly call MULTIQC_TSV_FROM_LIST
Small fixes to remove dummy files and params calls in nf-core/modules
Remove duplicate variants in ARTIC ONT pipeline
@github-actions
Copy link

github-actions bot commented Feb 3, 2022

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 2304a5c

+| ✅ 131 tests passed       |+
#| ❔   5 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.2
  • Run at 2022-02-04 17:19:39

Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Just made some minor comments. Awesome work guys! 👏

]
}

withName: 'MULTIQC_TSV_FAIL_READS' {
Copy link
Member

Choose a reason for hiding this comment

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

I guess here is not a problem since the MULTIQC not aliased process does indeed set the same path, but maybe it would be better to alias it to MULTIQC_MAIN to avoid any issue?

conf/modules_nanopore.config Show resolved Hide resolved
@@ -4,139 +4,157 @@
"repos": {
Copy link
Member

Choose a reason for hiding this comment

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

You will love this one 👀 , there is a new version of all these modules after this PR has been merged nf-core/modules#1261

Copy link
Member Author

Choose a reason for hiding this comment

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

Bah, it can wait for the next release now that we have tested everything. Didn't even see that it was merged! Thought we were still discussing it 👀

@JoseEspinosa
Copy link
Member

@drpatelh found this warnings when running NXF_VER=21.10.3 nextflow run main.nf -profile test_nanopore,docker --gff false:

WARN: There's no process matching config selector: .*:.*:.*:.*:TABIX_BGZIP
WARN: There's no process matching config selector: .*:.*:.*:.*:.*:TABIX_TABIX
WARN: There's no process matching config selector: .*:.*:.*:.*:.*:BCFTOOLS_STATS

A [`nextclade dataset`](https://docs.nextstrain.org/projects/nextclade/en/latest/user/datasets.html#nextclade-datasets) feature was introduced in [Nextclade CLI v1.3.0](https://github.com/nextstrain/nextclade/releases/tag/1.3.0) that fetches input genome files such as reference sequences and trees from a central dataset repository. We have uploaded Nextclade dataset [v2022-01-18](https://github.com/nextstrain/nextclade_data/releases/tag/2022-01-24--21-27-29--UTC) to [nf-core/test-datasets](https://github.com/nf-core/test-datasets/blob/viralrecon/genome/MN908947.3/nextclade_sars-cov-2_MN908947_2022-01-18T12_00_00Z.tar.gz?raw=true), and for reproducibility, this will be used by default if you specify `--genome 'MN908947.3'` when running the pipeline. However, there are a number of ways you can use a more recent version of the dataset:

* Supply your own by setting: `--nextclade_dataset <PATH_TO_DATASET>`
* Let the pipeline create and use the latest version by setting: `--nextclade_dataset false --nextclade_dataset_tag false`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest not including this as an option, as it allows for non-reproducibility

Copy link
Member Author

Choose a reason for hiding this comment

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

We have had a number of users that just want to use the latest version and this allows them to do that 🤷🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦

input_file.eachLine { line ->
def val = line.split(sep)[col]
if (uniqify) {
if (!vals.contains(val)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would make my algorithms teacher upset, better to use a Set in this case. Will suggest this change elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

This will have to wait until the next release now! Minor change :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, no problem. There are more low hanging fruit than this for improving performance 😉

Use ft,sb filters for Fisher's exact test and strand bias
Copy link
Contributor

@heuermh heuermh left a comment

Choose a reason for hiding this comment

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

Nice work!

@drpatelh
Copy link
Member Author

drpatelh commented Feb 4, 2022

@drpatelh found this warnings when running NXF_VER=21.10.3 nextflow run main.nf -profile test_nanopore,docker --gff false:

No way around this I'm afraid. It's because we initialise params.gff in main.nf and not nextflow.config so it isn't accessible to use in modules.config hence the warnings. We need to find a better solution to this problem. It's quite deeprooted.

@drpatelh
Copy link
Member Author

drpatelh commented Feb 4, 2022

Thanks guys!

@drpatelh drpatelh merged commit fc9fece into master Feb 4, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants