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

Issue 509 & 313 #533

Merged
merged 14 commits into from
May 4, 2022
Merged

Issue 509 & 313 #533

merged 14 commits into from
May 4, 2022

Conversation

FriederikeHanssen
Copy link
Contributor

@FriederikeHanssen FriederikeHanssen commented May 3, 2022

Closes #509
closes #313

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 - add to the software_versions process and a regex to scrape_software_versions.py
    • 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 (nextflow run . -profile test,docker).
  • 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).

workflows/sarek.nf Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

Easy to do indeed.
Should we have some tests for that?

Co-authored-by: Maxime U. Garcia <maxime.garcia@scilifelab.se>
@FriederikeHanssen
Copy link
Contributor Author

Easy to do indeed. Should we have some tests for that?

Probably a good idea. Let me add some

workflows/sarek.nf Outdated Show resolved Hide resolved
workflows/sarek.nf Outdated Show resolved Hide resolved
Comment on lines 2 to 5
test,XX,0,sample1,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test.paired_end.recalibrated.sorted.cram,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test.paired_end.recalibrated.sorted.cram.crai
test1,XX,1,sample2,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test2.paired_end.recalibrated.sorted.cram,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test2.paired_end.recalibrated.sorted.cram.crai
test3,XX,0,sample3,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test.paired_end.recalibrated.sorted.cram,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test.paired_end.recalibrated.sorted.cram.crai
test3,XX,1,sample4,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test2.paired_end.recalibrated.sorted.cram,https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/genomics/homo_sapiens/illumina/cram/test2.paired_end.recalibrated.sorted.cram.crai
Copy link
Member

Choose a reason for hiding this comment

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

why 2 pairs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used to make sure that sample3 is not run. I think I am missing a couple of check sthough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So basically I justed wanted to make sure we can test all options of the channel magic, that is why two "pairs". Single-normal, single-tumor, and a paired one. Or do you think that is too extensive?

Copy link
Member

@maxulysse maxulysse May 4, 2022

Choose a reason for hiding this comment

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

Ooooh I see.
May I suggest an alternative solution, not sure if it'll work or not, but could be worth a try
Have 3 different samplesheets:

  • single normal
  • single tumor
  • pair

That way we can use those in different tests.
but call it here in this case with --input samplesheet1.csv,samplesheet2.csv,samplesheet3.csv
I know it worked on local with previous sarek version, might be working here as well in this case

Copy link
Member

Choose a reason for hiding this comment

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

otherwise, happy with this idea, that's good and smart

Copy link
Member

Choose a reason for hiding this comment

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

you can stop looking, managed to run:

nextflow run . -profile test,docker --input "tests/csv/3.0/fastq_*.csv"

There's no , involved, just * so won't work on https path, just s3 + local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, ok what works locally now is nextflow run main.nf -profile test,variantcalling_channels,docker --input tests/csv/3.0/recalibrated_{germline,tumoronly,somatic}.csv --tools strelka

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it think it is the if (params.input) csv_file = file(params.input) line and how this is expanded https://www.nextflow.io/docs/latest/script.html#files-and-i-o

Copy link
Member

Choose a reason for hiding this comment

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

yes, we can use globs, so * or {}, I'd say probably ?, but not https definitively

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah never mind actually doesn't run the processes except for the first file.

@FriederikeHanssen
Copy link
Contributor Author

ok one last thing, there is actually a recalibrated.csv that has a bunch of file and almost fulfills all constraints. I would tweak that one for this use case ( we are not using it so far for CI tests)

@FriederikeHanssen FriederikeHanssen changed the title Issue 509 Issue 509 & 313 May 4, 2022
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

pure beauty

@FriederikeHanssen FriederikeHanssen merged commit 9a6b79a into nf-core:dev May 4, 2022
@FriederikeHanssen FriederikeHanssen deleted the issue_509 branch July 10, 2023 19:00
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