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.

Sign up for GitHub

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

Jump to bottom

Pseudo PR - DO NOT MERGE #19

Closed
wants to merge 217 commits into from
Closed

Pseudo PR - DO NOT MERGE #19

wants to merge 217 commits into from

Conversation

ewels
Copy link
Member

@ewels ewels commented Apr 8, 2021

Pseudo-PR for review of full pipeline prior to initial release, as described in https://nf-co.re/developers/adding_pipelines#core-pipeline-review

Do not merge - this PR just provides an interface to look over and comment on all pipeline code. Any suggestions can be applied to the dev branch and this PR will automatically update. Once it has two community reviews, preferably one from someone in @nf-core/core , it can be closed and release 1.0 can go.

ypriverol and others added 30 commits January 19, 2021 20:29
@ypriverol
change README
78a2c83
@ypriverol
Merge pull request #1 from bigbio/master
6d7b52b 
change README
@ypriverol
configuration files added.
e7c0c45
@ypriverol
Updated the README.md
5618bde
@ypriverol
Merge branch 'dev' of https://github.com/bigbio/pgdb into dev
e37e167
@ypriverol
Updated the README.md
f1cb026
@ypriverol
Updated the README.md
9a278c3
@ypriverol
increase the version of the pipeline.
f3de69f
@ypriverol
increase the version of the pipeline.
8eec9e1
@ypriverol
config paths solved
7653d86
@ypriverol
updated the .githubignore
87ece60
@ypriverol
change the biotypes to params -> bug fixed
14246ce
@ypriverol
decoy = false
5b06e8b
@ypriverol
change .gitignore
82891a5
@ypriverol
change .gitignore
049713d
@ypriverol
fix error with nextflow get_software_versions
ff97aab
@ypriverol
Merge pull request #2 from nf-core/dev
f365b1c 
Major changes in the Nf-core pipeline
@ypriverol
new ci example
ac90b87
@ypriverol
new ci example cosmic variants
22bd0db
@ypriverol
new ci example cosmic variants
90a50b4
@ypriverol
small change in ci
d3adc43
@ypriverol
small change in ci
e1d77db
@ypriverol
small change in ci
2215a46
@ypriverol
update package version. -> 0.0.10
69cd53c
@ypriverol
Merge pull request #3 from nf-core/dev
5ff76fc 
major changes in fork
@ypriverol
fix bug in cosmic
e964076
@ypriverol
disable cosmic tests, to big in files
54ddc23
@husensofteng
Process cosmic cell line data properly
fe93d19 
when either cosmic or cosmic_celllines are true, do:
 - download cosmic data
 - extract cosmic data
@ypriverol
update in py-pgatk version -> 0.0.11
a4f2b3d
@ypriverol
update usage
7a84202
ewels and others added 27 commits April 15, 2021 00:43
@ewels
Space after // comment
e843382
@ewels
Drop duplicate .Rprofile creation from Dockerfile
6d08b56
@ewels
--taxonomy not used anywhere, replaced in schema and nextflow.config …
37b2fd6 
…for --ensembl_name
@ewels
Finish stripping custom_runName
bb32a88
@ewels
Use the pipeline summary from the schema
04e34f0
@ypriverol
Merge pull request #28 from ewels/main-nf-whitespace
9607a8e 
Main nf whitespace
@ypriverol
Merge branch 'dev' of https://github.com/nf-core/pgdb into dev
bd21676 
� Conflicts:
�	docs/usage.md
�	main.nf
@ypriverol
small changes
b3a4075
@ypriverol
small changes
f8ea73f
@ypriverol
small changes
4f65b9f
@ypriverol
input skip test
39a11a5
@ypriverol
remove the intermedia files
9414d02
@ypriverol
remove the intermedia files
b2e80c9
@ypriverol
remove the intermedia files
137f9e1
@ypriverol
Merge pull request #29 from ypriverol/dev
f0a76d0 
minor changes to respond to #19
@ErikDanielsson
Merged changes from nf-core template
169537c
@ypriverol
Merge pull request #31 from ErikDanielsson/merging-template-updates
11ab4ae 
Merging template updates
@maxulysse
feat: remove social preview image to use GitHub OpenGraph
7b66698
@ewels
Merge pull request #33 from maxulysse/dev
dc85bd2 
Remove social preview image to use GitHub OpenGraph
@ewels
Fix up markdown in output.md for cleaner rendering & ToC.
dbeb5d8
@ewels
Remove COSMIC user details from test ci.yml
defbeda
@ypriverol
Merge pull request #34 from ewels/master
b06ebf4 
First release review tweaks
@ewels
Refactor to use publishDir to save final output files instead of manu…
affcf2d 
…al copyTo
@ewels
Use closure for decoy publishDir
d73aeb2
@ypriverol
Merge pull request #35 from ewels/master
cccb6e8 
Use `publishDir` to save final output files instead of manual `copyTo`
@ewels
Remove params.push_s3, minor typos
2bc2d17
@ypriverol
Merge pull request #36 from ewels/master
25eb864 
Remove params.push_s3, minor typos
@ewels
Copy link
Member Author

ewels commented Apr 26, 2021

Ok, I think we're about there! 🚀

Final remaining stuff that I don't love:

  • Usage docs aren't bundled with the pipeline or in nf-core, really. They're at https://pgatk.readthedocs.io/en/latest/workflows.html
    • In my experience, if the docs are not with the pipeline then they tend to drift away from reality quite quickly. So would be better if these could be in usage.md if possible.
  • The docs have improved a lot, but I think that there is still room for improvement. Notably with longer help texts for the parameters. For example - what do the three decoy methods do? How do they differ? When should I choose which one? Basically just trying to make the docs complete enough that someone like me can walk up knowing nothing and figure everything out by just reading the nf-core website.
  • I'm not sure what the add_reference_proteome process does and have a sneaking suspicion that it can be removed with no effect
    • As far as I can tell it just does a cat on one file to another with a different name and outputs that. If I've not missed something then maybe you can just skip it and use ensembl_protein_database_sub directly.
    • I know that the >> appends, but as the processes are isolated I can’t see how it ever appends more than just the one file.

This stuff would be nice to address, but none of it is a deal-breaker. So this pipeline gets a big fat ✅ from me to go for initial release 🚀 👍🏻

Many thanks for your patience and responsiveness 🙇🏻

@ewels ewels closed this Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ypriverol ypriverol ypriverol approved these changes

@jpfeuffer jpfeuffer jpfeuffer approved these changes

Assignees
No one assigned
Labels
WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ewels @ypriverol @jpfeuffer @husensofteng @nf-core-bot @ErikDanielsson @maxulysse