tabix/tabix: update module to support region-based VCF extraction by luisas · Pull Request #11064 · nf-core/modules

luisas · 2026-03-27T08:49:50Z

Add support for region-based VCF extraction

PR checklist

- Reverts tabix/tabix to its original indexing behaviour - New tabix/extract module: takes a bgzipped VCF + tbi + optional regions file - Outputs bgzipped VCF (always) and tbi index (optional, controlled by create_index input) - Follows samtools/view pattern for optional outputs

Always runs extraction + bgzip + tabix index. Both vcf and tbi outputs are optional: true — callers use whichever channels they need.

- Drop tabix/extract — functionality merged into tabix/tabix - New input: optional regions file (pass [] to use indexing mode) - New input: index path (required for extraction mode) - index output: optional, emitted in indexing mode - vcf output: optional, emitted in extraction mode - tbi output: optional, emitted in extraction mode

famosab

Ok I started this review but then I realized that this is in my eyes a different module. Maybe @nf-core/maintainers have the same opinion but I would rather have this separate because from tabix/tabix I just expect indexing and I would otherwise call it tabix/extract or something similar.

Edit: I just saw that that was you original idea and now I am a little confused what led to the change :D

famosab · 2026-03-27T14:52:09Z

modules/nf-core/tabix/tabix/main.nf

+    tuple val(meta),  path(tab)
+    tuple val(meta2), path(regions)


Please make this into one input tuple :)

Suggested change

tuple val(meta), path(tab)

tuple val(meta2), path(regions)

tuple val(meta), path(tab), path(regions)

They are actually 2 different files that I believe conceptually work better in 2 tuples: the first one is the input file (with its own meta) and the second one is the subsetting instructions file (also should probably have its own meta) - do you think I could leave it separate?

One might want to do sample specific subsetting, which is much easier with 1 tuple.

It's easier to join inputs then it is to pass two inputs together in sync 😉

This is one of the reasons we are moving to a flatter input structure.

Oh ok! Fixing it then :)

modules/nf-core/tabix/tabix/main.nf

luisas · 2026-03-27T16:38:55Z

Hi @famosab, thanks for starting the review!

I started doing it like tabix_extract but then I had a chat with @FriederikeHanssen on how to handle these modules that have different optional outputs depending on the input flags and because of maintainance burdens we thought it would be better to have it in one module? Similar to samtools/view

I am torn tbh, I am also liking the tabix/extract version.

famosab · 2026-03-30T09:09:14Z

I get the point! I am at times unsure maybe we need to discuss this also in the maintainers meeting.

If I follow your arguments I would then vote for the other submodules of tabix being deprecated (because what makes them different "enough") or add this as a separate module. But I would find it confusing that some things are excluded and some are part of the "base" module. Do you get what I mean? Maybe I am a bit strict on this matter but I want to have a structure that is understandable and sort of follows the same arguments? We could also have a vote or ask for more opinions in the slack and then adjust all tabix modules? (sorry for making this bigger than it is)

luisas · 2026-03-30T10:06:23Z

I get the point! I am at times unsure maybe we need to discuss this also in the maintainers meeting.

If I follow your arguments I would then vote for the other submodules of tabix being deprecated (because what makes them different "enough") or add this as a separate module. But I would find it confusing that some things are excluded and some are part of the "base" module. Do you get what I mean? Maybe I am a bit strict on this matter but I want to have a structure that is understandable and sort of follows the same arguments? We could also have a vote or ask for more opinions in the slack and then adjust all tabix modules? (sorry for making this bigger than it is)

I see the point. Doing tabix/extract was also my original intuition but I also see the other side of having something like samtools/view. Pinging @FriederikeHanssen @adamrtalbot so we can decide how to proceed

luisas · 2026-03-31T08:16:24Z

I get the point! I am at times unsure maybe we need to discuss this also in the maintainers meeting.
If I follow your arguments I would then vote for the other submodules of tabix being deprecated (because what makes them different "enough") or add this as a separate module. But I would find it confusing that some things are excluded and some are part of the "base" module. Do you get what I mean? Maybe I am a bit strict on this matter but I want to have a structure that is understandable and sort of follows the same arguments? We could also have a vote or ask for more opinions in the slack and then adjust all tabix modules? (sorry for making this bigger than it is)

I see the point. Doing tabix/extract was also my original intuition but I also see the other side of having something like samtools/view. Pinging @FriederikeHanssen @adamrtalbot so we can decide how to proceed

@famosab given the discussion on the team-maintainers team in slack there seems not to be the general solution for every module but there is a general agreement on trying to keep options in the same module.

Here there is also 2 other modules under tabix but they are using different command lines (bgzip, bgzip + tabix, tabix)
This structure to me makes sense as it keeps a motivated separation (which tools or combination of tools is used) but then groups all the different command lines and outputs under each of these modules.

What do you think?

famosab

Few more requests, I am fine with keeping this in tabix/tabix

modules/nf-core/tabix/tabix/meta.yml

modules/nf-core/tabix/tabix/main.nf

famosab · 2026-03-31T08:46:06Z

modules/nf-core/tabix/tabix/tests/main.nf.test

+        then {
+            assertAll (
+                { assert process.success },
+                { assert snapshot(sanitizeOutput(process.out, unstableKeys: ["vcf"])).match() }


Can we not use nft-vcf to test the md5sum of the vcf?

I did not know about that - thank you! Will add now

modules/nf-core/tabix/tabix/tests/main.nf.test

Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>

luisas · 2026-03-31T13:30:07Z

@famosab ok now it should be all done!

Thank you a lot for the help!

luisas added 8 commits March 27, 2026 09:47

tabix/tabix: update module to support region-based VCF extraction

283cabb

tabix/extract: make both vcf and tbi outputs optional

f6c4dac

Always runs extraction + bgzip + tabix index. Both vcf and tbi outputs are optional: true — callers use whichever channels they need.

add tabix/tabix

24f25b5

update tests

39c5bd6

fix test

b54b602

fix downstream tests

d5d6fa1

luisas requested a review from maxulysse as a code owner March 27, 2026 13:53

update snapshot

0be1e03

famosab reviewed Mar 27, 2026

View reviewed changes

add tai input for the regions

44dd019

luisas added 2 commits March 27, 2026 18:05

emit bgzipped

067bffc

add regions to the same input channel

116bfca

famosab reviewed Mar 31, 2026

View reviewed changes

famosab and others added 9 commits March 31, 2026 10:47

Merge branch 'master' into tabix-tabix-update

03a80c4

Update modules/nf-core/tabix/tabix/meta.yml

958601e

Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>

Update modules/nf-core/tabix/tabix/main.nf

b238a9e

Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>

Update modules/nf-core/tabix/tabix/meta.yml

c8f6234

Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>

Update modules/nf-core/tabix/tabix/main.nf

e689684

Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>

Update modules/nf-core/tabix/tabix/tests/main.nf.test

349c817

Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>

add ntf-vcf

56846d4

add regions

288fee0

add new test

54a15c2

	tuple val(meta), path(tab)
	tuple val(meta2), path(regions)
	tuple val(meta), path(tab), path(regions)

Conversation

luisas commented Mar 27, 2026

PR checklist

Uh oh!

famosab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

famosab Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

luisas Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

adamrtalbot Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

luisas Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luisas commented Mar 27, 2026

Uh oh!

famosab commented Mar 30, 2026

Uh oh!

luisas commented Mar 30, 2026

Uh oh!

luisas commented Mar 31, 2026

Uh oh!

famosab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

famosab Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

luisas Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luisas commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

famosab left a comment •

edited

Loading