Skip to content

Update saltshaker reports#856

Open
ieduba wants to merge 53 commits into
devfrom
saltshaker_reports
Open

Update saltshaker reports#856
ieduba wants to merge 53 commits into
devfrom
saltshaker_reports

Conversation

@ieduba

@ieduba ieduba commented May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • use updated saltshaker modules, which allows saltshaker to run even with empty input, so no logic is required after running mitosalt.
  • add customer ID to samples' saltshaker classification reports (previously they had no identifier at all, and customer ID is preferred by customers over sample ID).
  • display individual reports as tabs in the final html (previously were just printed one after the other).
  • remove find/concatenate module, previously used to concatenate sample reports.

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/raredisease 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>).
  • Ensure the test suite passes (nextflow run . -profile test_singleton,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).

@github-actions

github-actions Bot commented May 25, 2026 •
edited
Loading

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 596192c

+| ✅ 251 tests passed       |+
#| ❔   6 tests were ignored |#
!| ❗  10 tests had warnings |!
Details

❗ Test warnings:

  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • pipeline_todos - TODO string in CONTRIBUTING.md: Add any pipeline specific contribution guidelines here, such as coding styles, procedures, checklists etc.
  • pipeline_if_empty_null - ifEmpty(null) found in /home/runner/work/raredisease/raredisease/subworkflows/local/align_sentieon/main.nf: _ mq_metrics = SENTIEON_DATAMETRICS.out.mq_metrics.ifEmpty(null) // channel: [ val(meta), path(mq_metrics) ]
    _
  • pipeline_if_empty_null - ifEmpty(null) found in /home/runner/work/raredisease/raredisease/subworkflows/local/align_sentieon/main.nf: _ qd_metrics = SENTIEON_DATAMETRICS.out.qd_metrics.ifEmpty(null) // channel: [ val(meta), path(qd_metrics) ]
    _
  • pipeline_if_empty_null - ifEmpty(null) found in /home/runner/work/raredisease/raredisease/subworkflows/local/align_sentieon/main.nf: _ gc_metrics = SENTIEON_DATAMETRICS.out.gc_metrics.ifEmpty(null) // channel: [ val(meta), path(gc_metrics) ]
    _
  • pipeline_if_empty_null - ifEmpty(null) found in /home/runner/work/raredisease/raredisease/subworkflows/local/align_sentieon/main.nf: _ gc_summary = SENTIEON_DATAMETRICS.out.gc_summary.ifEmpty(null) // channel: [ val(meta), path(gc_summary) ]
    _
  • pipeline_if_empty_null - ifEmpty(null) found in /home/runner/work/raredisease/raredisease/subworkflows/local/align_sentieon/main.nf: _ aln_metrics = SENTIEON_DATAMETRICS.out.aln_metrics.ifEmpty(null) // channel: [ val(meta), path(aln_metrics) ]
    _
  • pipeline_if_empty_null - ifEmpty(null) found in /home/runner/work/raredisease/raredisease/subworkflows/local/align_sentieon/main.nf: _ is_metrics = SENTIEON_DATAMETRICS.out.is_metrics.ifEmpty(null) // channel: [ val(meta), path(is_metrics) ]
    _
  • schema_lint - Input mimetype is missing or empty
  • schema_description - No description provided in schema for parameter: hisat2

❔ Tests ignored:

  • files_exist - File is ignored: conf/modules.config
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-raredisease_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-raredisease_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-raredisease_logo_dark.png
  • modules_config - modules_config

✅ Tests passed:

Run details

  • nf-core/tools version 4.0.2
  • Run at 2026-06-09 11:52:37

@ieduba ieduba marked this pull request as ready for review May 26, 2026 13:25
fellen31
fellen31 requested changes May 26, 2026
View reviewed changes

@fellen31 fellen31 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work! And I think good to have all the HTML generation in one process.

Since you are writing code that groups together one or more samples into one process, I would really like to encourage you to add a test beforehand (preferably in a separate PR) to CALL_SV_MT with multiple samples so you can be confident that your changes work not only with one sample. If not a test with real data, then at least a stub test.

Comment thread modules/local/saltshaker_to_html/main.nf Outdated
Comment thread modules/local/saltshaker_to_html/main.nf Outdated
Comment thread modules/local/saltshaker_to_html/main.nf Outdated
Comment thread subworkflows/local/call_structural_variants/main.nf Outdated
Comment thread subworkflows/local/call_sv_MT/tests/main.nf.test
Comment thread subworkflows/local/call_sv_MT/main.nf Outdated
Comment thread subworkflows/local/call_sv_MT/main.nf Outdated
Comment thread subworkflows/local/call_sv_MT/main.nf Outdated
Comment thread bin/saltshaker_to_html.py
ramprasadn
ramprasadn reviewed Jun 8, 2026
View reviewed changes
Comment thread modules/local/saltshaker_to_html/main.nf
peterpru
peterpru reviewed Jun 9, 2026
View reviewed changes
Comment thread bin/saltshaker_to_html.py
Comment thread bin/saltshaker_to_html.py
ieduba and others added 3 commits June 9, 2026 12:41
@ieduba @peterpru
Update bin/saltshaker_to_html.py
62801ff 
Co-authored-by: Peter Pruisscher <57712924+peterpru@users.noreply.github.com>
@ieduba
empty lines in python per suggestion
32cbf9a
fix python linting
6fe7f95
fellen31
fellen31 reviewed Jun 9, 2026
View reviewed changes

@fellen31 fellen31 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since you are writing code that groups together one or more samples into one process, I would really like to encourage you to add a test beforehand (preferably in a separate PR) to CALL_SV_MT with multiple samples so you can be confident that your changes work not only with one sample. If not a test with real data, then at least a stub test.

Nice with new tests, would it be possible for you to add it in a separate PR to make it easier to see that it didn't work before, and that your new changes now work?

  • use updated saltshaker modules, which allows saltshaker to run even with empty input, so no logic is required after running mitosalt.
  • add customer ID to samples' saltshaker classification reports (previously they had no identifier at all, and customer ID is preferred by customers over sample ID).
  • display individual reports as tabs in the final html (previously were just printed one after the other).
  • remove find/concatenate module, previously used to concatenate sample reports.

As an exercise, which of these could have been added in separate PRs to simplify the reviewing and the complexity of the code changes? Tests in one PR? Saltshaker module updates in one? Customer ID in one?

Also wonder if this PR should be made towards dev or master?

Comment thread subworkflows/local/call_sv_MT/main.nf
Comment thread subworkflows/local/call_sv_MT/main.nf
Comment thread subworkflows/local/call_sv_MT/main.nf Outdated
Comment thread subworkflows/local/call_sv_MT/main.nf Outdated
Comment thread CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ramprasadn ramprasadn ramprasadn left review comments

@fellen31 fellen31 fellen31 requested changes

@peterpru peterpru peterpru approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ieduba @ramprasadn @fellen31 @peterpru