Skip to content

Template merge 3.3.1#379

Merged
jonasscheid merged 27 commits into
nf-core:devfrom
jonasscheid:template-merge-331
Jun 25, 2025
Merged

Template merge 3.3.1#379
jonasscheid merged 27 commits into
nf-core:devfrom
jonasscheid:template-merge-331

Conversation

@jonasscheid

@jonasscheid jonasscheid commented Jun 5, 2025

Copy link
Copy Markdown
Collaborator

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/mhcquant branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

Comment thread .github/workflows/nf-test.yml Outdated
Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>

@marissaDubbelaar marissaDubbelaar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two minor questions, also why are the tests not running through?
Can we ignore that for this PR?

Comment thread conf/test_full.config
variable_mods = 'Oxidation (M),Carbamidomethyl (C)'

// Pipeline settings
filter_mzml = true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this parameter be removed?

@jonasscheid jonasscheid Jun 12, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I still did not fully understand what happened, but if I don't remove the optional mzml filter, there are no spectra anymore and the pipeline resulted in having no actual results, just intermediate.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That sounds very dubious I must say 😅.
If the test can be ignored for now, then I can approve it from my side (if you still need approval at least)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think so.. See #379 (comment)
@marissaDubbelaar PR is ready to review :)

Comment thread nextflow.config
@jonasscheid

jonasscheid commented Jun 12, 2025

Copy link
Copy Markdown
Collaborator Author

There is some floating point rounding error going on that let nf-test fail.
e.g.
Column 16:
LEFT: 98.16315917968744
RIGHT: 98.16303710937495
Column 18:
LEFT: 1149.8631591796875
RIGHT: 1149.863037109375
Column 20:
LEFT: 98.16315917968744
RIGHT: 98.16303710937495

I'll adjust the test such that it checks for content

@jonasscheid

Copy link
Copy Markdown
Collaborator Author

@maxulysse do you know the plan for test_fullprofile? Is there a snapshot needed (for AWS tests on-release)?

@maxulysse

Copy link
Copy Markdown
Member

@maxulysse do you know the plan for test_fullprofile? Is there a snapshot needed (for AWS tests on-release)?

We only test that it launches and finish without failures.

@jonasscheid jonasscheid merged commit 0a4cbdb into nf-core:dev Jun 25, 2025
15 checks passed
@jonasscheid jonasscheid deleted the template-merge-331 branch June 25, 2025 07:52
@jonasscheid jonasscheid mentioned this pull request Sep 17, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants