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

Update Genrich module input tuple and argument #3720

Merged
merged 18 commits into from
Oct 18, 2023
Merged

Update Genrich module input tuple and argument #3720

merged 18 commits into from
Oct 18, 2023

Conversation

samuelruizperez
Copy link
Contributor

@samuelruizperez samuelruizperez commented Aug 10, 2023

This PR:

  • Joins treatment and control in tuple of input channels.
  • Makes it possible to input a list of treatment or control files, convert it to a comma-separated list, and analyse biological replicates simultaneously (see Genrich#multiple-replicates). A test using two treatments as input was added.
  • Adds -y (Keep unpaired alignments) as the default when meta.single_end:true (see Genrich#unpaired-alignments and Genrich/issues/#77).

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 module conventions in the contribution docs
  • Follow the naming conventions.
  • Follow the input/output options guidelines.
  • Follow the parameters requirements.
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware

@samuelruizperez samuelruizperez marked this pull request as ready for review August 10, 2023 13:23
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.

Generally I think this is fine, but I've never used the tool.

Question: Do we need all these value channels here? They only determine whether or not to save output, which could be controlled by ext.args.

def bed = save_bed ? "-b ${prefix}.intervals.bed" : ""
def args = task.ext.args ?: ""
def prefix = task.ext.prefix ?: "${meta.id}"
def layout = (!args.contains("-y") && meta.single_end) ? "-y" : ""
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this doing?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, would like also to know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single-end data can only be analyzed if unpaired alignments are kept (-y or -w <int>) so it defaults to -y if neither is already in args (jsh58/Genrich#77 (comment)). Should be clearer now.

def args = task.ext.args ?: ""
def prefix = task.ext.prefix ?: "${meta.id}"
def layout = (!args.contains("-y") && meta.single_end) ? "-y" : ""
def treatment = treatment_bam ? "-t ${treatment_bam.sort().join(',')}" : ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you sort here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, is it trying to maintain the correspondence between control and treatment 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.

Yes, is it trying to maintain the correspondence between control and treatment files?

That was my intention, but I don't think it's necessary (?)

Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Just drop two comments essentially the same as @SPPearce, would be great if you could address them in order to move nf-core/atacseq#331 on

def bed = save_bed ? "-b ${prefix}.intervals.bed" : ""
def args = task.ext.args ?: ""
def prefix = task.ext.prefix ?: "${meta.id}"
def layout = (!args.contains("-y") && meta.single_end) ? "-y" : ""
Copy link
Member

Choose a reason for hiding this comment

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

Yes, would like also to know

def args = task.ext.args ?: ""
def prefix = task.ext.prefix ?: "${meta.id}"
def layout = (!args.contains("-y") && meta.single_end) ? "-y" : ""
def treatment = treatment_bam ? "-t ${treatment_bam.sort().join(',')}" : ""
Copy link
Member

Choose a reason for hiding this comment

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

Yes, is it trying to maintain the correspondence between control and treatment files?

@samuelruizperez
Copy link
Contributor Author

Question: Do we need all these value channels here? They only determine whether or not to save output, which could be controlled by ext.args.

Removed the optional value channels.

@SPPearce
Copy link
Contributor

I need to review properly on my computer not my phone, but looks better at first pass.

Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀
You just need to remove this line and lint will pass and we should be ready to merge once all tests pass

@samuelruizperez samuelruizperez removed the request for review from drpatelh October 17, 2023 18:45
@@ -35,11 +35,11 @@
- path: output/genrich/test.intervals.bed
md5sum: 4bea65caa3f4043d703af4b57161112e
- path: output/genrich/test.narrowPeak
md5sum: d41d8cd98f00b204e9800998ecf8427e
md5sum: 82bc06c2e44e4d91152a6ac6557a2c6e
Copy link
Member

Choose a reason for hiding this comment

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

Even better 😍

@samuelruizperez samuelruizperez added this pull request to the merge queue Oct 18, 2023
Merged via the queue into nf-core:master with commit 53cb517 Oct 18, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants