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

Getting mapped bam and bai published in the same folder #1541

Merged
merged 5 commits into from
May 27, 2024
Merged

Getting mapped bam and bai published in the same folder #1541

merged 5 commits into from
May 27, 2024

Conversation

asp8200
Copy link
Contributor

@asp8200 asp8200 commented May 23, 2024 •
edited
Loading

Issue #1540

QUESTION : Should I add a pytest covering this bug? (It should be super easy to add such a test using the below-mentioned ntest-cmd.)

The following test works on this PR:

nextflow run main.nf -profile test,alignment_to_fastq,docker,debug --outdir results --save_mapped --save_output_as_bam

and gives

├── mapped
│   └── test
│       ├── test.sorted.bam
│       └── test.sorted.bam.bai

I also tried fixing the problem by setting

https://github.com/nf-core/sarek/blob/b5b766d3b4ac89864f2fa07441cdc8844e70a79e/conf/modules/aligner.config#L52C21-L52C52

to ) { "mapped/${meta.sample}/${it}" }, but then I got

results/preprocessing/mapped/<meta.sample>/<meta.id>.sorted.bam
results/preprocessing/mapped/<meta.sample>/<meta.id>.sorted.bam.bai

that is, the above-mentioned test gave:

├── mapped
│   └── test
│       ├── test-1.sorted.bam
│       └── test-1.sorted.bam.bai

I suggest that we add the above-mentioned test as a pytest.

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/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test tests/ --verbose --profile +docker).
  • 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).

@github-actions GitHub Actions
Copy link

github-actions bot commented May 23, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 01fe57d

+| ✅ 197 tests passed       |+
#| ❔  15 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: conf/modules.config
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/WorkflowSarek.groovy
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
  • template_strings - template_strings
  • modules_config - modules_config

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-05-27 09:46:31

@asp8200 asp8200 marked this pull request as ready for review May 23, 2024 16:13
@asp8200 asp8200 requested review from FriederikeHanssen and removed request for FriederikeHanssen May 23, 2024 16:13
@maxulysse
Copy link
Member

Changelog?

@asp8200
Copy link
Contributor Author

asp8200 commented May 23, 2024 •
edited
Loading

Changelog?

@maxulysse : Sure, but what about adding a pytest covering this bug, so that we don't reintroduce the bug later?
How do we test that this is also working when meta.size * meta.num_lanes != 1 ?

@asp8200 asp8200 mentioned this pull request May 24, 2024
Merged
11 tasks
@asp8200
Copy link
Contributor Author

asp8200 commented May 27, 2024

Changelog?

@maxulysse : Sure, but what about adding a pytest covering this bug, so that we don't reintroduce the bug later? How do we test that this is also working when meta.size * meta.num_lanes != 1 ?

@maxulysse : I ran

nextflow run main.nf -profile test,docker,debug --outdir results --save_mapped --save_output_as_bam

where we - as expected - get meta.num_lanes = 2 and also in that case we get published

├── mapped
│   └── test
│       ├── test.sorted.bam
│       └── test.sorted.bam.bai

maxulysse
maxulysse previously approved these changes May 27, 2024
View reviewed changes
FriederikeHanssen
FriederikeHanssen reviewed May 27, 2024
View reviewed changes
workflows/sarek/main.nf
Comment on lines +256 to +261
reads_for_alignment = reads_for_alignment.map{ meta, reads ->
// Update meta.id to meta.sample no multiple lanes or splitted fastqs
if (meta.size * meta.num_lanes == 1) [ meta + [ id:meta.sample ], reads ]
else [ meta, reads ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

can this not be moved up to the previous mapping statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean, but you're welcome to move stuff around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, the other is Adam's cool grouping logic. One more concern from my side, below there is the joining between the reads_grouping_key and reads_alignment meta's. Is this working for both splitting and unsplitting and across several lanes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea 😬

You mean these combine-statements:

https://github.com/asp8200/sarek/blob/8d1592c242fbb2bd2cc7da66377032d55afc7816/workflows/sarek/main.nf#L270

https://github.com/asp8200/sarek/blob/8d1592c242fbb2bd2cc7da66377032d55afc7816/workflows/sarek/main.nf#L288

I just hope we have CI-tests covering all this intricate stuff 🤞

Copy link
Member

Choose a reason for hiding this comment

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

#1544

Copy link
Member

Choose a reason for hiding this comment

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

How about we merge as is for now, and we'll take care of beautifying it when we refactor?

@maxulysse maxulysse mentioned this pull request May 27, 2024
Open
@asp8200 asp8200 requested a review from maxulysse May 27, 2024 10:01
@maxulysse maxulysse merged commit 76367cc into nf-core:dev May 27, 2024
20 checks passed
@asp8200 asp8200 deleted the fix_1540 branch June 4, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FriederikeHanssen FriederikeHanssen FriederikeHanssen left review comments

@maxulysse maxulysse maxulysse 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
@asp8200 @maxulysse @FriederikeHanssen