Fix: Update Cellranger/multi to reinclude functionality from previous PR update#12152
Fix: Update Cellranger/multi to reinclude functionality from previous PR update#12152mahesh-panchal wants to merge 4 commits into
Conversation
|
@leandrotiburske @fmalmeida The PR is ready and I think I have the flags in, but please check. From today I won't be near a computer to make updates so I made this branch on nf-core modules where you can both push/pull your own changes onto this branch. The tests on this module look like they really need updating as well, but that's perhaps outside the scope. I leave this up to you both now. |
|
This PR should remove the old python template too since it's redundant. |
|
Hi @leandrotiburske, Do you think you can take over from here to do the final amendments? As @mahesh-panchal mentioned, the try is to move all the logic into the module itself. So, could you double check if all the functionality from before is now there so to remove the template script? As Mahesh, I am also not fluent in this tool, I had only helped out on the code but someone else was assessing the results ... so I believe it would be really good if someone that knows how the results should be could take a closer look. I am on holidays until next Wednesday and I only have a phone, so I cannot help with the code until there but I can help with comments and review. |
nictru
left a comment
nictru
left a comment
There was a problem hiding this comment.
Some minor comments
Also: The python script is not being removed by this PR so far
| def create_bam = gex_has_opts.containsKey("create-bam") ? gex_has_opts["create-bam"] : "true" | ||
| gex_section << "create-bam,${create_bam}" | ||
| // create-bam defaults to true if not specified | ||
| gex_section << "create-bam,${gex_options?.get('create-bam') ?: 'true'}" |
There was a problem hiding this comment.
If gex_options?.get('create-bam') returns boolean false, then string true will be used here. Is this something that could become a real problem? Or can one be sure that gex_options?.get('create-bam') will always return a string 'false'?
| if [ "${skip_renaming}" = "true" ]; then | ||
| r1=\$(find "\${r1_dir}" -maxdepth 1 -name "*.fastq.gz" | head -1) | ||
| r2=\$(find "\${r2_dir}" -maxdepth 1 -name "*.fastq.gz" | head -1) | ||
| [ -z "\${r1}" ] || [ -z "\${r2}" ] && continue |
There was a problem hiding this comment.
From what I can see the old version raised an AssertionError on mismatch, here we skip silently
Yeah. I don't think it is ready yet. It still has things to be done, as with the script you mentioned. Tagged you because it would be really good to have a review of someone that understands the underlying data better than me. Once I am back from holidays I can give it a check to try to finish it if not progressed until then. |
3 participants
Description
Related to #11066 . Adds back in functionality from the python template that was accidentally dropped.
PR checklist
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile condanf-core subworkflows test <SUBWORKFLOW> --profile dockernf-core subworkflows test <SUBWORKFLOW> --profile singularitynf-core subworkflows test <SUBWORKFLOW> --profile conda