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

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

Draft
wants to merge 1,702 commits into
base: first-commit-for-pseudo-pr
Choose a base branch
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.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CITATIONS.md Outdated Show resolved Hide resolved
CITATIONS.md Show resolved Hide resolved
CITATIONS.md Outdated Show resolved Hide resolved
subworkflows/local/prepare_genome.nf Outdated Show resolved Hide resolved
workflows/circrna.nf Outdated Show resolved Hide resolved
workflows/circrna.nf Outdated Show resolved Hide resolved
workflows/circrna.nf Outdated Show resolved Hide resolved
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... ;)

bin/circ_test.R Outdated Show resolved Hide resolved
bin/circ_test.R Outdated Show resolved Hide resolved
bin/circ_test.R Outdated Show resolved Hide resolved
bin/circ_test.R Outdated Show resolved Hide resolved
bin/circ_test.R Outdated Show resolved Hide resolved
@@ -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' ..

conf/test.config Outdated Show resolved Hide resolved
// 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 ?

docs/output.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
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.

8 participants