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

Update spring/decompress-module #1573

Merged
merged 10 commits into from
Jul 8, 2024
Merged

Update spring/decompress-module #1573

merged 10 commits into from
Jul 8, 2024

Conversation

asp8200
Copy link
Contributor

@asp8200 asp8200 commented Jun 19, 2024 •
edited
Loading

Updating the spring/decompress-module. This PR should change nothing with regards to published files, but the SPRING_DECOMPRESS was being called rather confusingly on this input (one sample with two spring-files - one with R1 and the other with R2).

In this test, dev-Sarek calls spring on the spring-file containing just the R1-reads (test_1.fastq.gz.spring):

spring \
        -d \
        -g \
        -t 2 \
         \
        -i test_1.fastq.gz.spring \
        -o test_1_R1.fastq.gz test_1_R2.fastq.gz

All reads go to test_1_R1.fastq.gz while test_1_R2.fastq.gz doesn't even get written.

That is not terrible, but then the same pipeline-run also contains the following call of SPRING_DECOMPRESS on test_2.fastq.gz.spring which contains all the R2-reads:

spring \
        -d \
        -g \
        -t 2 \
         \
        -i test_2.fastq.gz.spring \
        -o test_2_R1.fastq.gz test_2_R2.fastq.gz

Here, again, all reads go into test_2_R1.fastq.gz and still test_2_R2.fastq.gz isn’t written. That seems suboptimal to me.

With the updated module, we get

spring \
    -d \
    -g \
    -t 2 \
     \
    -i test_1.fastq.gz.spring \
    -o test_1.fastq.gz

and

    spring \
    -d \
    -g \
    -t 2 \
     \
    -i test_2.fastq.gz.spring \
    -o test_2.fastq.gz

which I find much more sensible.

TO-DO:

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 19, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 455b0a2

+| ✅ 200 tests passed       |+
#| ❔  12 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_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-07-01 14:02:29

@maxulysse
Copy link
Member

Can we instead update the modules upstream and add something like?

def output = meta.single_end || meta.one_strand ? "-o ${prefix}.fastq.gz" : "-o ${prefix}_R1.fastq.gz ${prefix}_R2.fastq.gz"

asp8200
asp8200 commented Jun 19, 2024
View reviewed changes
conf/modules/modules.config Outdated Show resolved Hide resolved
@asp8200
Copy link
Contributor Author

asp8200 commented Jun 19, 2024 •
edited
Loading

Can we instead update the modules upstream and add something like?

def output = meta.single_end || meta.one_strand ? "-o ${prefix}.fastq.gz" : "-o ${prefix}_R1.fastq.gz ${prefix}_R2.fastq.gz"

You mean use your definition of output in the version of spring/decompress over in nf-core/modules? (That is fine with me.)

@maxulysse
Copy link
Member

Can we instead update the modules upstream and add something like?

def output = meta.single_end || meta.one_strand ? "-o ${prefix}.fastq.gz" : "-o ${prefix}_R1.fastq.gz ${prefix}_R2.fastq.gz"

You mean use your definition of output in the version of spring/decompress over in nf-core/modules? (That is fine with me.)

No, but add meta.one_strand in for each spring file when there is a pair.

@asp8200
Copy link
Contributor Author

asp8200 commented Jun 19, 2024

Can we instead update the modules upstream and add something like?

def output = meta.single_end || meta.one_strand ? "-o ${prefix}.fastq.gz" : "-o ${prefix}_R1.fastq.gz ${prefix}_R2.fastq.gz"

You mean use your definition of output in the version of spring/decompress over in nf-core/modules? (That is fine with me.)

No, but add meta.one_strand in for each spring file when there is a pair.

Ok, I'll try if I can get that working. It should be possible.

@maxulysse
Copy link
Member

Because technically using meta.single_end would make sense, but we don't have single_end data, just the one strand with this file, so something like meta.one_strand would be better in our case.
But if you like one_direction so much we can argue in the PR on the modules repo.

@asp8200
Copy link
Contributor Author

asp8200 commented Jun 20, 2024

Because technically using meta.single_end would make sense, but we don't have single_end data, just the one strand with this file, so something like meta.one_strand would be better in our case. But if you like one_direction so much we can argue in the PR on the modules repo.

@maxulysse and @FriederikeHanssen : So I replaced ext.one_direction with meta.one_strand. Currently, I just did a local module patch, but - if you prefer - I can also update the module over in the modules-repo.

@maxulysse
Copy link
Member

Because technically using meta.single_end would make sense, but we don't have single_end data, just the one strand with this file, so something like meta.one_strand would be better in our case. But if you like one_direction so much we can argue in the PR on the modules repo.

@maxulysse and @FriederikeHanssen : So I replaced ext.one_direction with meta.one_strand. Currently, I just did a local module patch, but - if you prefer - I can also update the module over in the modules-repo.

I'd like an updated module in the modules repo.

I'd rather avoid patching modules as much as possible for future plans and reasons

@asp8200 asp8200 mentioned this pull request Jun 20, 2024
Merged
17 tasks
@asp8200
Copy link
Contributor Author

asp8200 commented Jun 20, 2024

Because technically using meta.single_end would make sense, but we don't have single_end data, just the one strand with this file, so something like meta.one_strand would be better in our case. But if you like one_direction so much we can argue in the PR on the modules repo.

@maxulysse and @FriederikeHanssen : So I replaced ext.one_direction with meta.one_strand. Currently, I just did a local module patch, but - if you prefer - I can also update the module over in the modules-repo.

I'd like an updated module in the modules repo.

I'd rather avoid patching modules as much as possible for future plans and reasons

nf-core/modules#5850

So like that?

@asp8200 asp8200 changed the title Update config of spring Update spring/decompress-module Jun 24, 2024
@FriederikeHanssen
Copy link
Contributor

Can we separate the spring modules stuff and whether sarek is able to handle a mixed bag of inputs? Can just have an issue were w collect the testing that has been done on that

@asp8200 asp8200 mentioned this pull request Jun 25, 2024
Open
@asp8200 asp8200 marked this pull request as ready for review June 25, 2024 09:02
@asp8200 asp8200 requested a review from maxulysse as a code owner June 25, 2024 09:02
FriederikeHanssen
FriederikeHanssen reviewed Jul 1, 2024
View reviewed changes
modules.json Outdated Show resolved Hide resolved
@FriederikeHanssen FriederikeHanssen self-requested a review July 8, 2024 13:45
@FriederikeHanssen FriederikeHanssen merged commit f475962 into nf-core:dev Jul 8, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse Awaiting requested review from maxulysse maxulysse is a code owner

@FriederikeHanssen FriederikeHanssen Awaiting requested review from FriederikeHanssen FriederikeHanssen is a code owner

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