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 a QUAST module #130

Merged
merged 13 commits into from
Feb 1, 2021
Merged

Adding a QUAST module #130

merged 13 commits into from
Feb 1, 2021

Conversation

KevinMenden
Copy link
Contributor

This PR adds a QUAST module.

To accommodate the two usecases of running quast with and without a reference genome, I ended up creating two modules, or "tools" for QUAST. I tried out a few things but as of now there's really no good way of dealing with optional inputs in DSL2. Until that's possibly, I think this is an adequate solution.

Let me know if you find any bugs or anything I should change :)

@drpatelh
Copy link
Member

drpatelh commented Feb 1, 2021

Thanks @KevinMenden . Ok, I think I have a solution that allows us to keep this repo clean from duplicating modules to deal with optional inputs. It's not ideal but possibly the best solution that allows developers to use their own parameter names e.g. instead of params.gff, params.fasta etc.

    input:
    path consensus
    path fasta
    path gff
    val use_fasta
    val use_gff

If we add two more boolean variables to the input then these can be evaluated in the script section to stage the appropriate file. This means we don't have to use params.gff anymore and keeps the module as clean as possible in terms of parameter names. Also, we get to keep a single instance of the module which I think is best given that this issue is really Nextflow specific. If/when optional inputs are supported we update the module. Simples.

What do you think?

@KevinMenden
Copy link
Contributor Author

Okay yes, it's the best we can do right now I reckon :/
I agree this is nicer than having two different 'modules' for quast.

The only thing I'm a bit worried is how the fasta and gtf paths are evaluated, we probably need to pass some dummy values to them that look look like paths (basically just ""). I will test around a bit.

Thanks @drpatelh ! Will ping you when I've changed it.

@drpatelh
Copy link
Member

drpatelh commented Feb 1, 2021

Cool. Yup, that is exactly what I do if params.gff hasn't been provided. See here. This ensures that a file is always staged regardless of whether it needs to be used.

@KevinMenden
Copy link
Contributor Author

@drpatelh okay everything refactored now :)

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Amazing! Thanks @KevinMenden . Few minor changes. I find it more intuitive grouping by variable type than by name.

software/quast/main.nf Outdated Show resolved Hide resolved
software/quast/main.nf Outdated Show resolved Hide resolved
software/quast/main.nf Outdated Show resolved Hide resolved
software/quast/meta.yml Outdated Show resolved Hide resolved
software/quast/meta.yml Outdated Show resolved Hide resolved
tests/software/quast/main.nf Outdated Show resolved Hide resolved
tests/software/quast/main.nf Outdated Show resolved Hide resolved
KevinMenden and others added 7 commits February 1, 2021 14:02
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
@KevinMenden
Copy link
Contributor Author

Alright thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new module Adding a new module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants