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

Adding rdeval #6778

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

Conversation

richard-burhans
Copy link
Contributor

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Thanks @richard-burhans. A few comments inline.

#if $output_options.output_type.type_selector == "combined_reads"
ln -s '$reads_outfile' 'output.${output_type.format_selector}' &&
#end if
rdeval --input-reads #echo " ".join([f"'{input}'" for $input in $mangled_inputs])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a simple for loop is more readable and does not need the echo magic?

<param argument="--md5" type="boolean" checked="false" label="Print md5 of .rd files"/>
</when>
</conditional>
<param argument="--verbose" type="boolean" checked="false" label="Verbose output"/>
Copy link
Member

Choose a reason for hiding this comment

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

I would not offer this as a param for the user. Activate it by default, it does not hurt I guess.

Copy link
Member

Choose a reason for hiding this comment

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

@richard-burhans do you need this param exposed to the user?

</inputs>
<outputs>
<data name="stats_outfile" format="tabular" label="Rdeval summary"/>
<data name="rd_outfile" from_work_dir="output.rd" format="binary" label="RD File">
Copy link
Member

Choose a reason for hiding this comment

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

What kind of format is that? Will it be used by downstream tools?

Copy link
Member

Choose a reason for hiding this comment

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

ping ...

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Seems to be part of gfastats.. Should we add it herehttps://github.com/bgruening/galaxytools/blob/master/tools/gfastats/gfastats.xml?

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

To me it seems that we already have good tools for these tasks. What would be the advantage of having this tool?

Please do not get me wrong. Your implementation of the Galaxy tool is fine, but adding it to iuc comes with a long term commitment in maintenance. And I do not (yet see this justified).

@@ -0,0 +1,205 @@
<tool id="rdeval" name="rdeval" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" profile="@PROFILE@">
<description>Multithreaded read analysis and manipulation tool</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Description is quiet technical and at the same time non descriptive.

</param>
<when value="no_file"/>
<when value="exclude_file">
<param argument="--exclude-list" type="data" format="txt" optional="true" label="File containing headers to exclude"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional makes no sense in the conditional. Maybe skip the conditional and just have the two parameters as optional?

<param argument="--include-list" type="data" format="txt" optional="true" label="File containing headers to include"/>
</when>
</conditional>
<param argument="--filter" type="text" optional="true" label="filter" help="e.g. l&gt;1000 &amp; q&gt;20"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear. Should we really offer it this way too users?

<section name="input_subsample" title="Subsample input reads" expanded="false">
<param argument="--sample" type="float" min="0" max="1" value="1" label="fraction of reads to subsample"/>
<param argument="--random-seed" type="integer" min="0" optional="true" label="random seed to make subsampling reproducible"/>
<param argument="--homopolymer-compress" type="integer" min="0" optional="true" label="Compress homopolymers longer than n in the input"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

</param>
</when>
<when value="size">
<param argument="--out-size" type="select" optional="true" label="size list type">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what these options mean?

</when>
</conditional>
<conditional name="output_type">
<param name="type_selector" type="select" label="output type">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

<change_format>
<when input="format_selector" value="fasta.gz" format="fasta.gz"/>
<when input="format_selector" value="fastq.gz" format="fastq.gz"/>
<when input="format_selector" value="bam" format="bam"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This allows conversion from fasta to bam?

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