Skip to content

Add mosdepth - #227

Open
lazappi wants to merge 6 commits into
mainfrom
feature/add-mosdepth
Open

Add mosdepth#227
lazappi wants to merge 6 commits into
mainfrom
feature/add-mosdepth

Conversation

@lazappi

@lazappi lazappi commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

Add a component for mosdepth, a tool for fast calculation of coverage depth.

Checklist before requesting a review

  • I have performed a self-review of my code

  • Conforms to the Contributing guidelines

  • Proposed changes are described in the CHANGELOG.md

  • I have tested my code with viash ns test --parallel -q <name or namespace>

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Documentation
    • Bug fixes

@lazappi
lazappi marked this pull request as ready for review July 28, 2026 07:02
@lazappi
lazappi requested a review from mdgrv July 28, 2026 07:02
type: string
description: |
Restrict `--output_regions`/`--output_region_dist` output to windows
of this fixed size (an integer), or to the intervals in a BED file.

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.

Restrict --output_regions/--output_region_dist output to windows of this fixed size (an integer). To use a BED file of regions instead, pass it to --by_bed. One of --by/--by_bed is required in order to
use --thresholds.

Restrict `--output_regions`/`--output_region_dist` output to windows
of this fixed size (an integer), or to the intervals in a BED file.
Required in order to use `--thresholds`.
example: 500

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.

    example: 500
  - name: "--by_bed"
    type: file
    description: |
      BED file of regions to restrict `--output_regions`/
      `--output_region_dist` output to, as an alternative to the fixed-window
      `--by`. Mutually exclusive with `--by`. This is a separate file-typed
      argument (rather than reusing `--by`) so it is staged into the
      container.
    example: regions.bed

staged_fasta="$par_fasta"
fi
fi

@mdgrv mdgrv Jul 29, 2026

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.

# mosdepth's --by accepts either a fixed window size (integer) or a BED file.
# The BED case is exposed as a separate file-typed argument (--by_bed) so it is
# staged into the container; whichever is set is passed to mosdepth's --by.
by_value="$par_by"
if [ -n "$par_by_bed" ]; then
  if [ -n "$par_by" ]; then
    echo "Error: --by and --by_bed are mutually exclusive." >&2
    exit 1
  fi
  by_value="$par_by_bed"
fi

cmd_args=(
${meta_cpus:+--threads "$meta_cpus"}
${par_chrom:+--chrom "$par_chrom"}
${par_by:+--by "$par_by"}

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.

${by_value:+--by "$by_value"}

check_file_exists "$meta_temp_dir/test9_summary.txt" "Summary output (auto-discovered index)"
check_file_not_empty "$meta_temp_dir/test9_summary.txt" "Summary output (auto-discovered index)"
log "✅ TEST 9 completed successfully"

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.

##############################################################
log "Starting TEST 10: BED-file regions via --by_bed"
##############################################################
printf 'chr1\t100\t300\tregionA\n' >"$meta_temp_dir/regions.bed"
"$meta_executable"
--input "$bam"
--input_index "$bai"
--by_bed "$meta_temp_dir/regions.bed"
--no_per_base
--output_regions "$meta_temp_dir/test10_regions.bed.gz"
--output_regions_index "$meta_temp_dir/test10_regions.bed.gz.csi"

check_file_exists "$meta_temp_dir/test10_regions.bed.gz" "Regions output (--by_bed)"
check_file_not_empty "$meta_temp_dir/test10_regions.bed.gz" "Regions output (--by_bed)"
check_file_exists "$meta_temp_dir/test10_regions.bed.gz.csi" "Regions output index (--by_bed)"
gunzip -c "$meta_temp_dir/test10_regions.bed.gz" >"$meta_temp_dir/test10_regions.bed"
check_file_contains "$meta_temp_dir/test10_regions.bed" "regionA" "Regions output (--by_bed)"
log "✅ TEST 10 completed successfully"

@mdgrv mdgrv 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.

The component is clean and follows the biobox conventions
throughout (script rules, pinned container + software_versions.txt, ASCII, CHANGELOG
cites the PR). viash ns test is green and the numbers are correct on real data: on an
HG002 HiFi chr21 BAM, mosdepth's summary mean depth matches samtools coverage
(23.28 vs 23.2758).

One blocking issue, found on real data:

--by with a BED file fails under the Docker runner. --by is type: string, so a
BED file passed to it is never staged into the container - viash only mounts
type: file arguments. mosdepth then errors before reading any coverage:

[E::hts_open_format] Failed to open file ".../regions.bed" : No such file or directory

The integer-window form of --by works (a number needs no file), and CRAM works
(--fasta is type: file), so only the BED-interval form is affected. It's
data-independent - it reproduces on the shipped synthetic test BAM too - and the current
tests don't catch it because TEST 2/5 only pass an integer (--by 100).

Suggested fix: expose the BED case as a separate file-typed argument (--by_bed) so
viash mounts it, and route whichever of --by/--by_bed is set to mosdepth's --by.
Committable suggestions below, plus a test that passes a real BED. I applied and
validated exactly this in a local worktree: viash ns test passes (now 10 tests), the
real-data --by_bed run returns chr21 15000000 35000000 54.31 (== samtools 54.3115),
integer --by still works, and --by + --by_bed together errors as expected.

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.

2 participants