Added delly/filter, changed delly/call - this time with up-to-date fork

[alexnater]

PR checklist

Closes #4808

• This comment contains a description of changes (with reason).
• Added module delly/filter
• Changed module delly/call to allow somatic SV calling
• Changed testing to nf-test for delly/call
• If you've added a new tool - have you followed the module conventions in the contribution docs
• If necessary, include test data in your PR.
• Remove all TODO statements.
• Emit the versions.yml file.
• Follow the naming conventions.
• Follow the parameters requirements.
• Follow the input/output options guidelines.
• Add a resource label
• 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:
  • For modules:
    • nf-core modules test <MODULE> --profile docker
    • nf-core modules test <MODULE> --profile singularity
    • nf-core modules test <MODULE> --profile conda

[nvnieuwk]

LGTM 🚀

[nvnieuwk]

You should also remove the pytest files of delly/call and remove the module from pytest_modules.yml so the pytest tests don't run

[alexnater]

Done. The assertions might need some more works, as some files contain time stamps and md5 sums will probably not match.

[nvnieuwk]

You could check parts of the VCF content with path(process.out.<name_of_output>[0][1].linesGzip[-5..-1]) (This will snapshot the last 5 lines of the VCF). For the index you can simple snapshot the filename like this: file(process.out.<name_of_output>[0][1]).name

[alexnater]

The use of file and path in the test assertions is a bit confusing to me (file -> Nextflow, path -> Groovy?). I hope you actually meant path(process.out.<name_of_output>[0][1]).linesGzip[-5..-1] or this is not making any sense to me.

[nvnieuwk]

The use of file and path in the test assertions is a bit confusing to me (file -> Nextflow, path -> Groovy?). I hope you actually meant path(process.out.<name_of_output>[0][1]).linesGzip[-5..-1] or this is not making any sense to me.

Yes that's what I meant, sorry 😁

[SPPearce]

Looks like this has some merge conflicts that need fixing before it can be merged in

[SPPearce]

Still need to fix the tests, as mentioned they are not stable.

[SPPearce]

@nvnieuwk , can you take a look at this PR please, given that you've just merged changes to delly call.

[nvnieuwk]

Yeah my bad sorry, I forgot this PR did mostly the same changes. I'll fix the conflicts

[nvnieuwk]

Okay conflicts fixed but here are some pointers:

1. The tests are still using the old params.test_data way of getting the test files, this should be updated to use params.modules_testdata_base_path instead.
2. There is a newer version of delly, so can you please update it in delly/filter too?

[SPPearce]

@alexnater , do you need help to get this finished off?

[alexnater]

@alexnater , do you need help to get this finished off?

Hi Simon. Sorry, I forgot a bit about this pending PR. Yes, I think I would need some help to finish this off. I'm a bit confused about this missing review by @projectoriented. Who is this?

[SPPearce]

@alexnater , do you need help to get this finished off?

Hi Simon. Sorry, I forgot a bit about this pending PR. Yes, I think I would need some help to finish this off. I'm a bit confused about this missing review by @projectoriented. Who is this?

@projectoriented is listed as the code owner, which presumably means they initially contributed the module. You don't need to wait for a review by them.

What help do you need?

[SPPearce]

Bump

[famosab]

Bump :)