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

Dupradar #154

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Dupradar #154

wants to merge 19 commits into from

Conversation

emmarousseau
Copy link
Collaborator

Description

Add DupRadar component

Checklist before requesting a review

  • I have performed a self-review of my code

  • Conforms to the Contributing guidelines

  • Proposed changes are described in the CHANGELOG.md

  • I have tested my code with viash ns test --parallel -q <name or namespace>

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Documentation
    • Bug fixes

@emmarousseau emmarousseau marked this pull request as ready for review September 23, 2024 09:25
Comment on lines 35 to 75
- name: Output
arguments:
- name: --output_dupmatrix
type: file
direction: output
description: path to output file (txt) of duplicate tag counts
example: dup_matrix.txt
- name: --output_dup_intercept_mqc
type: file
direction: output
description: path to output file (txt) of multiqc intercept value DupRadar
example: dup_intercept_mqc.txt
- name: --output_duprate_exp_boxplot
type: file
direction: output
default: duprate_exp_boxplot.pdf
description: |
Path to output file (pdf) of distribution of expression box plot
- name: --output_duprate_exp_densplot
type: file
direction: output
description: |
Path to output file (pdf) of 2D density scatter plot of duplicate tag counts
example: duprate_exp_densityplot.pdf
- name: --output_duprate_exp_denscurve_mqc
type: file
direction: output
description: |
Path to output file (pdf) of density curve of gene duplication multiqc
example: duprate_exp_density_curve_mqc.txt
- name: --output_expression_histogram
type: file
direction: output
description: |
Path to output file (pdf) of distribution of RPK values per gene histogram
example: expression_hist.pdf
- name: --output_intercept_slope
type: file
direction: output
description: output file (txt) with progression of duplication rate values
example: intercept_slope.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these are required: true.

Comment on lines 30 to 36
mv "$par_id"_dupMatrix.txt $par_output_dupmatrix
mv "$par_id"_dup_intercept_mqc.txt $par_output_dup_intercept_mqc
mv "$par_id"_duprateExpBoxplot.pdf $par_output_duprate_exp_boxplot
mv "$par_id"_duprateExpDens.pdf $par_output_duprate_exp_densplot
mv "$par_id"_duprateExpDensCurve_mqc.txt $par_output_duprate_exp_denscurve_mqc
mv "$par_id"_expressionHist.pdf $par_output_expression_histogram
mv "$par_id"_intercept_slope.txt $par_output_intercept_slope
Copy link
Contributor

Choose a reason for hiding this comment

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

This expects all output files to be generated/present which is in contrast with the required: false of those parameters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I will keep the default names from the original tool if they are not specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide a test scenario where not all output parameters have been provided.

required: true
description: Path to GTF annotation file.
- name: --paired
type: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be type: boolean_true?

src/dupradar/config.vsh.yaml Show resolved Hide resolved
argument_groups:
- name: Input
arguments:
- name: --id
Copy link
Contributor

Choose a reason for hiding this comment

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

What if this parameter is not set?

Comment on lines 12 to 17
input_bam <- args[1]
output_prefix <- args[2]
annotation_gtf <- args[3]
stranded <- as.numeric(args[4])
paired_end <- ifelse(args[5] == "true", TRUE, FALSE)
threads <- as.numeric(args[6])
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes more sense to combine script.sh and dupradar.r and directly call the R script instead of using the wrapper. But that would force us to change the dupradar.r code itself. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That could be good, it's pretty straightforward and I think it would simplify the component, I'll get on that.

Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left some R-related suggestions

src/dupradar/dupradar.r Outdated Show resolved Hide resolved
src/dupradar/dupradar.r Outdated Show resolved Hide resolved
src/dupradar/dupradar.r Outdated Show resolved Hide resolved
@rcannood
Copy link
Contributor

I only made suggestions for individual lines of code, but they should be applied to the whole script ☺️

@emmarousseau
Copy link
Collaborator Author

I only made suggestions for individual lines of code, but they should be applied to the whole script ☺️

Right, my bad! lmk if I missed anything now

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