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

Added new module: msisensorpro/msitumoronly #6350

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

bounlu
Copy link
Contributor

@bounlu bounlu commented Aug 26, 2024

This module enables tumor-only analysis for msisenspro (msisensor-pro pro)

@bounlu bounlu changed the title add msisensorpro/msitumoronly module Added new module: msisensorpro/msitumoronly Aug 26, 2024
@SPPearce
Copy link
Contributor

Can you please add some tests via nf-core modules test msisensorpro/msitumoronly

Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

Requires some tests before it can be merged.

@famosab famosab requested a review from a team as a code owner September 2, 2024 09:42
@famosab famosab requested review from sateeshperi and removed request for a team September 2, 2024 09:42
@famosab famosab mentioned this pull request Sep 2, 2024
17 tasks
@famosab
Copy link
Contributor

famosab commented Sep 2, 2024

You'll probably need to add another module (draft here: #6536) to create the baseline. Or you have to add a baseline file to the test-datasets for the tests to run. Afaik the scan is not enough to run msitumoronly afterwards.

@bounlu
Copy link
Contributor Author

bounlu commented Sep 2, 2024

As a default baseline for hg38, this file can be added.

@famosab
Copy link
Contributor

famosab commented Sep 2, 2024

I tried with your baseline and the test data from their repo and always got this error:

│     Program aborted:                                                                                                                                                                                                                                                │
│     Same reference genome file should be used in both scan and msi/pro/baseline steps!!!

Maybe you can find the correct files that should be used here.

@famosab
Copy link
Contributor

famosab commented Sep 2, 2024

The stub is also not working as expected:


│   Caused by:                                                                                                                                                                                                                                                        │
│     Missing output file(s) `null` expected by process `MSISENSORPRO_MSITUMORONLY (test)`                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                     │
│                                                                                                                                                                                                                                                                     │
│   Command executed:                                                                                                                                                                                                                                                 │
│                                                                                                                                                                                                                                                                     │
│     mkdir test                                                                                                                                                                                                                                                      │
│     mkdir test_dis                                                                                                                                                                                                                                                  │
│     mkdir test_all                                                                                                                                                                                                                                                  │
│     mkdir test_unstable                                                                                                                                                                                                                                             │
│                                                                                                                                                                                                                                                                     │
│     cat <<-END_VERSIONS > versions.yml                                                                                                                                                                                                                              │
│     "MSISENSORPRO_MSITUMORONLY":                                                                                                                                                                                                                                    │
│         msisensor-pro: $(msisensor-pro 2>&1 | sed -nE 's/Version:\sv([0-9]\.[0-9])/\1/ p')                                                                                                                                                                          │
│     END_VERSIONS    

@bounlu
Copy link
Contributor Author

bounlu commented Sep 3, 2024

I tried with your baseline and the test data from their repo and always got this error:

│     Program aborted:                                                                                                                                                                                                                                                │
│     Same reference genome file should be used in both scan and msi/pro/baseline steps!!!

Maybe you can find the correct files that should be used here.

Hi. Thanks for testing. This error is not related to the pipeline, but to msisensorpro. It seems you are using a bam file incompatible with the chromosome names in the baseline. It works for me if I choose the default GRCh38 in sarek.

Not sure about the stub failure as I haven't added it.

Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

A few comments.

  • The prefix can't have def before it if it is being used to define the output channels, as that makes it a local variable. Think that'll help with your tests.

  • Requirements for the environment.yml have changed, so I've updated that.

  • We are moving away from external links, even to other GitHub repositories, so it'd be good to use data in test datasets please


script:
def args = task.ext.args ?: ''
def prefix = task.ext.prefix ?: "${meta.id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def prefix = task.ext.prefix ?: "${meta.id}"
prefix = task.ext.prefix ?: "${meta.id}"

Copy link
Contributor

Choose a reason for hiding this comment

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

Just because I am curious: I always see it the other way around (so that you need two times def and otherwise you run into errors). Has this changed? Is there any part of the docs you can point me to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too, I simply copy pasted it from other modules indeed.


stub:
def args = task.ext.args ?: ''
def prefix = task.ext.prefix ?: "${meta.id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def prefix = task.ext.prefix ?: "${meta.id}"
prefix = task.ext.prefix ?: "${meta.id}"

description: File containing final report with all detected microsatellites, unstable somatic microsatellites, msi score
- output_dis:
type: file
description: File containing distribution results
Copy link
Contributor

Choose a reason for hiding this comment

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

These look like folders not files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are files indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then the stub needs to touch the files not mkdir them.

"timestamp": "2024-09-02T11:32:08.214756"
},
"human - cram": {
"content": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks completely empty? Probably because of the prefix issue

@@ -0,0 +1,7 @@
name: msisensorpro_msitumoronly
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: msisensorpro_msitumoronly

channels:
- conda-forge
- bioconda
- defaults
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- defaults

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.

3 participants