[Do not merge!] Pseudo PR for first release #133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 🙂 |
mashehu
commented
Aug 9, 2024
| with: | ||
| report_paths: "*.xml" | ||
|
|
||
| test_profile_download: |
There was a problem hiding this comment.
why do you do this as separate job?
There was a problem hiding this comment.
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.
Still have to come up with a cool schema, any suggestions?
|
Thank you @mashehu ! Still very helpful! |
nictru
reviewed
Aug 14, 2024
There was a problem hiding this comment.
Very nice code quality overall!
Some notes:
- For each module, you only have an
.nffile. This is generally sufficient and fine for local modules, but you could better bundle the module resources by using a module directory with amain.nfand 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.
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.
Comment on lines
+17
to
+18
| path "stats.txt", emit: ch_stats | ||
| path "versions.yml", emit: versions |
There was a problem hiding this comment.
Suggested change
| 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.
Maybe it would be better to have this in modules.config, for consistency with other modules
There was a problem hiding this comment.
Absolutely true, I have no Idea how that ended up in the process 😆
There was a problem hiding this comment.
Please check if you could use this module
There was a problem hiding this comment.
Good point, I will check 👍
There was a problem hiding this comment.
This workflow is quite long, splitting it up into multiple subworkflows could increase readability
There was a problem hiding this comment.
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Do not merge! This is a PR of dev compared to first release for whole-pipeline reviewing purposes. Changes should be made to
devand this PR should not be merged into first-commit-for-pseudo-pr!