-
Notifications
You must be signed in to change notification settings - Fork 5
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 #133
base: TEMPLATE
Are you sure you want to change the base?
Conversation
Move some operations to subworkflow
Typo and documentation suggestions Co-authored-by: Sabrina Krakau <[email protected]>
Co-authored-by: Sabrina Krakau <[email protected]>
…_check Peptide length additional checks
Fix params
Co-authored-by: Till E. <[email protected]>
Add tests
Co-authored-by: Sabrina Krakau <[email protected]>
Change NCBI Parameter to Nextflow Secret
remove testdata directory
Refactor PR: Sorting, gzip timestamps, reproducible ids
nf-test assertions
Bugfix/figure creation
prepare release 1.0.0
|
Thank you @mashehu !! Thats really helpful 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few small things I saw. not a complete review
with: | ||
report_paths: "*.xml" | ||
|
||
test_profile_download: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you do this as separate job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the pipeline optionally can download assemblies from Entrez using taxids, for this a registered account is needed (implemented as nextflow secrets). The GitHub secrets defined on the repository (used then in the process) are only available to PRs from nf-core/metapep, but not any forks! We decided before the download "path" is not tested at all, the tests run after merging the PR on dev, so if there is something failing it is not unnoticed.
@@ -3,14 +3,38 @@ | |||
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|||
## v1.0dev - [date] | |||
## v1.0.0 - [2022-01-20] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any plan for fun release names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have to come up with a cool schema, any suggestions?
Thank you @mashehu ! Still very helpful! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice code quality overall!
Some notes:
- For each module, you only have an
.nf
file. This is generally sufficient and fine for local modules, but you could better bundle the module resources by using a module directory with amain.nf
and adding the script as a template in the module directory, instead of having it inbin/
. This way you could get rid of all the argument parsing builerplate. - When running the test configuration I saw that most modules don't provide a
tag
. Providing them would help users understanding what's going on. - Related: You don't use a meta map at all. While this is generally acceptable as long as the modules are local, having them would make the pipeline more easily understandable for other nf-core developers. However this would require a lot of refactoring in the channel handling, so I think the current state is fine for now.
- I think your pipeline delivers quite some output that could be presented as a MultiQC report. Maybe you could implement this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use the tag
directive more frequently in your processes. It helps users identify which data already has already gone through a certain process and which has not.
Commenting at this process because it is actually executed multiple times in the test config. As you are not using the meta map, you could still derive a tag
from the file name.
path "stats.txt", emit: ch_stats | ||
path "versions.yml", emit: versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path "stats.txt", emit: ch_stats | |
path "versions.yml", emit: versions | |
path "stats.txt", emit: ch_stats | |
path "versions.yml", emit: versions |
'https://depot.galaxyproject.org/singularity/mulled-v2-0be74e7b0c2e289bc8098b1491baf4f181012b1c:a1635746bc2c13635cbea8c29bd5a2837bdd7cd5-0' : | ||
'biocontainers/mulled-v2-0be74e7b0c2e289bc8098b1491baf4f181012b1c:a1635746bc2c13635cbea8c29bd5a2837bdd7cd5-0' }" | ||
|
||
publishDir "${params.outdir}/figures", mode: params.publish_dir_mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better to have this in modules.config
, for consistency with other modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely true, I have no Idea how that ended up in the process 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if you could use this module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will check 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow is quite long, splitting it up into multiple subworkflows could increase readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get nf-test to run without it, do you know how I could solve this?
Whoops I messed up and my mouse jumped 😆
First of all thank you for reviewing! I will look into your comments and see how I can resolve them! 🙂
This is a good point and could maybe help with readability. As most of those scripts were developed as standalones, they were implemented as such. I will think about taking it into the refactoring for the metamap (see point 3), though maybe not for this release but a follow-up 1.1.!!
A good point, probably very helpful, for users not so proficient, I will implement some tags!
There was some development for the epitopeprediction modules within nf-core/epitopeprediction, and we wanted to align our prediction step, in this effort I would also implement the metamap, as well as refactor all module inputs, possibly as templates as mentioned above.
|
Did you want to add a comment on MultiQC? :D |
Do not merge! This is a PR of dev compared to first release for whole-pipeline reviewing purposes. Changes should be made to
dev
and this PR should not be merged into first-commit-for-pseudo-pr!