Dsl2-polyg-polyx-filtering - #1177
Open
ilight1542 wants to merge 9 commits into
Open
Conversation
…d erroneous help-text
This PR is against the
|
Author
|
related to #1175 |
|
|
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
TCLamnidis
requested changes
Aug 7, 2026
Collaborator
There was a problem hiding this comment.
Looking at the FastP documentation of the polyG trimming options, it seems by default it is on for 2-colour chemistry, and off for 4-colour chemistry.
The originally referenced option --trim_poly_g forces trimming on all datasets, while the newly referenced option --disable_trim_poly_g effectively turns off trimming for 2-colour chemistry.
I think it might make more sense to convert these parameters into a single parameter switch here. something like preprocessing_fatsp_polygtrimmode, which would be a choice between 'none', '2colour'(default), and 'all'? That would give users access to all the functionality, without forcing us to validate if and which incompatible options have been provided together.
| "type": "integer", | ||
| "default": 0, | ||
| "description": "Specify the complexity threshold that must be reached or exceeded to retain reads.", | ||
| "help_text": "This option can be used to define the minimum length of a poly-X (A,T,C,N) tail to begin low complexity trimming following poly-G trimming (if performed). This would be carried out on all sequencing data, regardless of the colour chemistry.\n\n> Modifies fastp parameter: `--trim_poly_x` and `--poly_x_min_len`", |
Collaborator
There was a problem hiding this comment.
If setting the parameter to 0 disables polyx filtering, it should be clearly stated.
Suggested change
| "help_text": "This option can be used to define the minimum length of a poly-X (A,T,C,N) tail to begin low complexity trimming following poly-G trimming (if performed). This would be carried out on all sequencing data, regardless of the colour chemistry.\n\n> Modifies fastp parameter: `--trim_poly_x` and `--poly_x_min_len`", | |
| "help_text": "This option can be used to define the minimum length of a poly-X (A,T,C,N) tail to begin low complexity trimming following poly-G trimming (if performed). This would be carried out on all sequencing data, regardless of the colour chemistry. A value of 0 means that no poly-X filtering is performed.\n\n> Modifies fastp parameter: `--trim_poly_x` and `--poly_x_min_len`", |
Collaborator
There was a problem hiding this comment.
Also, the FastP docs have slightly confusing phrasing about this. Does this happen AFTER poly-G or before poly-G trimming?
I think your helptext is correct, but it is worth double-checking.
This setting is useful for trimming the tails having polyX (i.e. polyA) before polyG.
| params.preprocessing_skipadaptertrim ? "--disable_adapter_trimming" : params.preprocessing_adapterlist ? "" : params.preprocessing_adapter1 ? "--adapter_sequence ${params.preprocessing_adapter1}" : "", | ||
| "--length_required ${params.preprocessing_minlength}", | ||
| params.preprocessing_fastp_complexityfilter ? "--low_complexity_filter --complexity_threshold ${params.preprocessing_fastp_complexityfilter_threshold}" : "", | ||
| params.preprocessing_fastp_disable_polygfilter ? "--disable_trim_poly_g" : "${meta.colour_chemistry}" == 4 ? "--trim_poly_g --poly_g_min_len ${params.preprocessing_fastp_polygfilter_minlength}" : "--disable_trim_poly_g", |
Collaborator
There was a problem hiding this comment.
Doesn't this turn on polyG filtering only for 4-colour chem, but force it off for 2-colour chem? 🤔 Maybe that's why the tests were failing?
Either way, I think this should be restructured to use a single option for enable/disable, as described in the review.
The min length parameter can then be independent from this logic and always set (except if FastP complains when min length is set while disabling the filter).
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Adjustments to parameter texts and invocation within modules.config to allow for poly-g disabling and adjustment of length.
corrections:
--preprocessing_fastp_disable_polygfilter--preprocessing_fastp_polygfilter_minlength--preprocessing_fastp_polyxfilter_minlengthabove 0PR checklist
scrape_software_versions.pynf-core lint .).nextflow run . -profile test,docker).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).