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.

Sign up for GitHub

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

Jump to bottom

Fix unite urls #766

Merged
merged 16 commits into from
Jul 24, 2024
Merged

Fix unite urls #766

merged 16 commits into from
Jul 24, 2024

Conversation

erikrikarddaniel
Copy link
Member

@erikrikarddaniel erikrikarddaniel commented Jul 24, 2024 •
edited
Loading

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
  • If necessary, also make a PR on the nf-core/ampliseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

Urls for Unite databases updated, plus addition of some newly released versions.

I added a test for ITS sequences with DADA taxonomy, but they take a lot of memory so I didn't add to what's automatically run.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 24, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 8e9b28d

+| ✅ 285 tests passed       |+
#| ❔   6 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-07-24 17:26:42

@d4straub
Copy link
Collaborator

Great!
Did you test also some QIIME2 runs just to make sure? If not, I can do that.
test_sintax seems to have a problem with the data and the db.

@erikrikarddaniel
Copy link
Member Author

erikrikarddaniel commented Jul 24, 2024 •
edited
Loading

Great! Did you test also some QIIME2 runs just to make sure? If not, I can do that. test_sintax seems to have a problem with the data and the db.

I did, but only with test_its_dada_taxonomy and --qiime_ref_taxonomy. I used --skip_qiime to save time with all other tests, which is why I didn't notice this error before submitting the PR. The Ancom process SUMMARY_REPORT complains about a collision of file names, but there also seems to be a problem with the Ancom process. I have no idea, do you?

@d4straub
Copy link
Collaborator

Thats also a mystery to me, I can look into that, but unfortunately not immediately.

@erikrikarddaniel
Copy link
Member Author

Thats also a mystery to me, I can look into that, but unfortunately not immediately.

See Slack message.

d4straub
d4straub reviewed Jul 24, 2024
View reviewed changes
modules/local/qiime2_ancom_tax.nf Outdated
@@ -43,7 +43,7 @@ process QIIME2_ANCOM_TAX {
--to-tsv

if [ \$(grep -v '^#' -c ${table.baseName}-level-${taxlevel}.feature-table.tsv) -lt 2 ]; then
echo ${taxlevel} > ancom/\"WARNING Summing your data at taxonomic level ${taxlevel} produced less than two rows (taxa), ANCOM can't proceed -- did you specify a bad reference taxonomy?\".txt
echo ${taxlevel} > ancom/"${table.baseName}_WARNING_Summing_your_data_at_taxonomic_level_${taxlevel}_produced_less_than_two_rows_(taxa)_ANCOM_cant_proceed_--_did_you_specify_a_bad_reference_taxonomy.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prepending ${table.baseName} unfortunately defeats its purpose, because the warning wont be picked up by

ampliseq/subworkflows/local/qiime2_ancom.nf

Line 45 in e66c9d3

QIIME2_ANCOM_TAX.out.ancom.subscribe { if ( it.baseName[0].toString().startsWith("WARNING") ) log.warn it.baseName[0].toString().replace("WARNING ","QIIME2_ANCOM_TAX: ") }
any more. Would need to be adjusted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I'll put it after the WARNING.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it.baseName[0].toString().replace("WARNING ","QIIME2_ANCOM_TAX: ") will still not work, because its now WARNING_ instead of WARNING (mind the space), correct? Other than that the PR seem fine to me.

Copy link
Member Author

@erikrikarddaniel erikrikarddaniel Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I reverted most of this, just keeping the table.baseName in the filename. (I don't think we have a test case for this any more, since I corrected the Sintax urls.)

d4straub
d4straub approved these changes Jul 24, 2024
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@erikrikarddaniel
Copy link
Member Author

@nf-core-bot fix linting

@erikrikarddaniel erikrikarddaniel merged commit e057eba into nf-core:dev Jul 24, 2024
17 checks passed
@erikrikarddaniel erikrikarddaniel mentioned this pull request Jul 24, 2024
Closed
@d4straub d4straub mentioned this pull request Jul 25, 2024
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d4straub d4straub d4straub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erikrikarddaniel @d4straub @nf-core-bot