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

Wrong display values for "zymo" and "em_seq" presets on help page #335

Closed
wants to merge 3 commits into from
Closed

Wrong display values for "zymo" and "em_seq" presets on help page #335

wants to merge 3 commits into from

Conversation

wassimsalam01
Copy link
Contributor

PR checklist

  • This comment contains a description of changes (with reason).

Changes:

  1. Updated "--clip_r2" value in "zymo" from 15 to 10
  2. Updated all preset values of "em_seq" from 8 to 10

I am currently working with data where I had to use the "zymo" trimming preset. I was immensely confused when I saw discrepancies between the expected displayed behavior on the parameters help page and the actual results I was obtaining.

So I cross-checked the values for the presets between the methylseq Parameters help page and the Bismark Bisulfite Mapper documentation, on top of actually testing the preset in a run, and found that the value for "--clip_r2" should be 10 and not 15 on the help page.

This further compelled me to check and test the other presets, where I found through the same checking methods that the preset values for "em_seq" are also wrongly displayed. They should be 10 and not 8.

sateeshperi and others added 3 commits June 12, 2023 14:25
@sateeshperi
Merge pull request #321 from nf-core/dev
81f989c 
Dev -> Master 2.4.0
@wassimsalam01
Update nextflow_schema.json
bb2857f 
Updated zymo preset "--clip_r2" argument value from 15 to 10.
@wassimsalam01
Update nextflow_schema.json
2f458e4 
Updated all em_seq preset argument values from 8 to 10.
@wassimsalam01 wassimsalam01 changed the base branch from master to dev August 17, 2023 01:10
@github-actions
Copy link

github-actions bot commented Aug 17, 2023 •
edited
Loading

nf-core lint overall result: Failed ❌

Posted for pipeline commit 2f458e4

+| ✅ 146 tests passed       |+
#| ❔   3 tests were ignored |#
!| ❗   1 tests had warnings |!
-| ❌   8 tests failed       |-

❌ Test failures:

  • nextflow_config - Config variable not found: params.validationShowHiddenParams
  • nextflow_config - Config variable not found: params.validationSchemaIgnoreParams
  • files_unchanged - .github/CONTRIBUTING.md does not match the template
  • files_unchanged - .github/ISSUE_TEMPLATE/bug_report.yml does not match the template
  • files_unchanged - assets/nf-core-methylseq_logo_light.png does not match the template
  • files_unchanged - lib/NfcoreTemplate.groovy does not match the template
  • schema_params - Param schema_ignore_params from nextflow config not found in nextflow_schema.json
  • multiqc_config - 'assets/multiqc_config.yml' does not contain a matching 'report_comment'.

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-08-17 01:28:32

@wassimsalam01
Copy link
Contributor Author

wassimsalam01 commented Aug 17, 2023 •
edited
Loading

@nf-core-bot fix linting

@nf-core nf-core deleted a comment from github-actions bot Aug 24, 2023
edmundmiller
edmundmiller approved these changes Aug 25, 2023
View reviewed changes
Copy link
Collaborator

@edmundmiller edmundmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

I double-checked the logic in the config, just to make sure it matches up as well. @FelixKrueger wrote it, so hopefully that part is right 😆

Hot take: @FelixKrueger we should make a json or yaml or csv or something of the table linked above, and then parse it either in the methylseq pipeline, or get it baked right into trimgalore, so we could do trimgalore --em-seq...

Not sure how that'll ever fix our schema being out of sync issue, though.

@edmundmiller edmundmiller mentioned this pull request Aug 25, 2023
Open
@edmundmiller edmundmiller added this to the 2.5.0 milestone Aug 25, 2023
@edmundmiller edmundmiller added the bug Something isn't working label Aug 25, 2023
edmundmiller added a commit that referenced this pull request Aug 25, 2023
@edmundmiller
Merge pull request #335 from update_kit_schema
16007cf 
Wrong display values for "zymo" and "em_seq" presets on help page
@edmundmiller
Copy link
Collaborator

Merged locally because the branch started off master, and figuring out how to fix that isn't something I'd wish on my worst enemy.

@wassimsalam01
Copy link
Contributor Author

wassimsalam01 commented Aug 28, 2023 •
edited
Loading

Thank you Edmund 😄 yeah apologies for that haha, I rushed it and hadn't paid attention to that detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmundmiller edmundmiller edmundmiller approved these changes

@SPPearce SPPearce Awaiting requested review from SPPearce

@ewels ewels Awaiting requested review from ewels

Assignees

@wassimsalam01 wassimsalam01

Labels
bug Something isn't working
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wassimsalam01 @edmundmiller @sateeshperi