8000 [Do not merge!] Pseudo PR for first release by jfy133 · Pull Request #69 · nf-core/circrna · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Do not merge!] Pseudo PR for first release #69

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

Closed
wants to merge 1,773 commits into from

Conversation

jfy133
Copy link
Member
@jfy133 jfy133 commented Jan 23, 2023

Do not merge! This is a PR of dev compared to first release for whole-pipeline reviewing purposes.

Comments can be copied over to #66 for safety if necessary and changes made there. Changes should be made to dev and this PR should not be merged into first-commit-for-pseudo-pr!

@jfy133 jfy133 marked this pull request as draft January 23, 2023 07:05
Copy link
Member Author
@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, main things are:

  • Add license to ALL scripts in bin/ (particularly for the package)
  • Make sure all version numbers are defined for all tools in the conda declaration of local modules with mulled containers,and these are exported.

ch_multiqc_files = ch_multiqc_files.mix(ch_workflow_summary.collectFile(name: 'workflow_summary_mqc.yaml'))
ch_multiqc_files = ch_multiqc_files.mix(ch_methods_description.collectFile(name: 'methods_description_mqc.yaml'))
ch_multiqc_files = ch_multiqc_files.mix(CUSTOM_DUMPSOFTWAREVERSIONS.out.mqc_yml.collect())
ch_multiqc_files = ch_multiqc_files.mix(FASTQC_TRIMGALORE.out.fastqc_zip.collect{it[1]}.ifEmpty([]))
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it correct only FastQC goes into MultiQC?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll look into generating BAM stats

README.md Outdated

**workflow for the quantification, differential expression analysis and miRNA target prediction analysis of circRNAs in RNA-Seq data**.
[![AWS CI](https://img.shields.io/badge/CI%20tests-full%20size-FF9900?labelColor=000000&logo=Amazon%20AWS)](https://nf-co.re/circrna/results)[![Cite with Zenodo](http://img.shields.io/badge/DOI-10.5281/zenodo.XXXXXXX-1073c8?labelColor=000000)](https://doi.org/10.5281/zenodo.XXXXXXX)

Choose a reason for hiding this comment

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

@jfy133 @apeltzer can we figure this out before the relase? Otherwise, another Zenodo ID is coming ;) Barry, the docs for this were recently updated. Shout if you need help to add the ID after the release. Someone from core (Alex) needs to generate this beforehand for you.

README.md Outdated
Comment on lines 34 to 37
4. circRNA annotation
5. Export mature spliced length as FASTA file
6. Annotate parent gene, underlying transcripts.
7. circRNA count matrix

Choose a reason for hiding this comment

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

Are these the custom R scripts? Otherwise maybe also link the tools here in a similar fashion to above

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, vanilla python and bash.

section_name: "nf-core/circrna Methods Description"
section_href: "https://github.com/nf-core/circrna"
plot_type: "html"
## TODO nf-core: Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline

Choose a reason for hiding this comment

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

you might want to add your paper here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll give it a go! I went to the Sarek one to copy but... ;)

@@ -0,0 +1,18 @@
species command
cel useMart(biomart = "ensembl", dataset = "celegans_gene_ensembl", host="https://www.ensembl.org", archive=FALSE)

Choose a reason for hiding this comment

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

This fetches the newest version every time, right? I am a bit worried about reproducibility and compatibility in the long run. Would there be a way to cache it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Think I'll deprecate this code, its only going to cause trouble

}
}

// PREPARE GENOME

Choose a reason for hiding this comment

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

I suspect you will get WARN messages if the indices already exist

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll test this. I have a hard time resolving WARN messages


// PREPARE GENOME
withName: BOWTIE_BUILD {
ext.when = { params.fasta && !params.bowtie && params.tool.split(',').contains('mapsplice') && params.module.split(',').contains('circrna_discovery') }

Choose a reason for hiding this comment

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

what happens if params.tools doesn't exist? Or does it always have to have a value?

Copy link
@FriederikeHanssen FriederikeHanssen Jan 27, 2023

Choose a reason for hiding this comment

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

similar for params.module

Copy link
Collaborator
@BarryDigby BarryDigby Jan 31, 2023

Choose a reason for hiding this comment

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

must have a value, a la --tool 'mutect2', --step 'mapping' ..

// Input data for full size test
// TODO nf-core: Specify the paths to your full test data ( on nf-core/test-datasets or directly in repositories, e.g. SRA)
// TODO nf-core: Give any required params for the test so that command line flags are not needed
input = 'https://raw.githubusercontent.com/nf-core/test-datasets/viralrecon/samplesheet/samplesheet_full_illumina_amplicon.csv'

Choose a reason for hiding this comment

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

is this the right input?

Copy link
Collaborator

Choose a reason for hiding this comment

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

nope, I was hoping for some help in getting my full test dataset up and running.

@@ -1,63 +1,590 @@
# nf-core/circrna: Output

## :warning: Please read this documentation on the nf-core website: [https://nf-co.re/circrna/output](https://nf-co.re/circrna/output)

Choose a reason for hiding this comment

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

is this line removed in the template @jfy133 ?

@lpantano
Copy link

@FriederikeHanssen , @nictru , just checking this is work in progress?

@nictru
Copy link
Collaborator
nictru commented Mar 11, 2025

The story of this pipeline is a bit unsatisfying:
Basically Barry Digby wrote the pipeline and published a paper on it, but never finalized the 1.0 release. Half a year after he left, I took over the development. I am working towards starting a new 1.0 release effort, but there are some features that I would like to add before that.

This PR is still open from the time before I took over development. Actually I did not dare to close it, not sure what the official thing to do is in this situation.

@lpantano
Copy link

Ok, then I will close it and you can start the release when you are ready. Thanks!

@lpantano lpantano closed this Mar 12, 2025
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.

9 participants
0