Plasmodium _subset_genome_sequence_region has the same truthiness bug

[Gopisokk]

Summary

PlasmodiumDataResource._subset_genome_sequence_region() uses truthiness checks on region.start and region.end to decide whether to slice the genome sequence. Because Python evaluates 0 as falsy, specifying region.start = 0 silently skips slicing and returns the entire contig instead of the requested sub-region.

This is the same class of bug as #940, but in a completely separate code path (plasmodium.py) that is not covered by that issue.

Affected Location

File: malariagen_data/plasmodium.py, lines 241–248

def _subset_genome_sequence_region(
    self, genome, region, inline_array=True, chunks="native"
):
    """Sebset reference genome sequence."""
    region = self._resolve_region(region)
    z = genome[region.contig]

    d = _da_from_zarr(z, inline_array=inline_array, chunks=chunks)

    if region.start:           # ← BUG: 0 is falsy
        slice_start = region.start - 1
    else:
        slice_start = None
    if region.end:             # ← BUG: 0 is falsy
        slice_stop = region.end
    else:
        slice_stop = None
    loc_region = slice(slice_start, slice_stop)

    return d[loc_region]

Expected Behaviour

When region.start = 0 is specified, data should be sliced from position 0. The value 0 is a valid genomic coordinate.

Actual Behaviour

Slicing is skipped. slice_start is set to None, so d[None:] returns the entire contig as if no region was specified. No error or warning is raised.

Why This Is Hard to Catch

• The failure is silent — no exception, no warning
• Most real-world queries don't start at position 0
• The returned data is valid and well-formed, just the wrong amount of it
• Downstream analysis runs to completion on the inflated dataset

Proposed Fix

Replace the truthiness checks with explicit None checks:

if region.start is not None:
    slice_start = region.start - 1
else:
    slice_start = None
if region.end is not None:
    slice_stop = region.end
else:
    slice_stop = None

Additional fix in the same function

The docstring on line 235 has a typo: "Sebset" → "Subset".

Relationship to #940

Issue #940 covers 7 instances of the same truthiness bug in snp_data.py and hap_data.py. This issue covers the separate occurrence in plasmodium.py, which uses a different code pattern (separate if blocks instead of if ... or ...) and is in a completely different class (PlasmodiumDataResource vs. the Anopheles data classes).

Happy to submit a PR for this!

[khushthecoder]

hi, @jonbrenas

Why this issue is critical:

This is a silent data correctness bug — the most dangerous category. The affected functions return data that looks correct but is silently wrong when region.start = 0. Since the library uses 1-based genomic coordinates, start=0 with the old code would produce slice_start = -1 (Python treats this as "from the end"), returning garbage data with no error or warning.

Even beyond the start=0 case, the truthiness check means that start=None and start=0 are treated identically — a semantic confusion that violates the principle of least surprise.

Scope beyond #1108:

While investigating the reported bug in plasmodium.py, I discovered two additional occurrences of the same anti-pattern:

1. AnophelesGenomeSequenceData.genome_sequence() in genome_sequence.py — identical truthiness bug with resolved_region.start and resolved_region.end
2. AnophelesSnpData.is_accessible() in snp_data.py (line 1726) — truthiness check on resolved_region.start for computing the offset

I fixed all three in this PR for completeness, since they are the exact same class of bug.

My approach:

1. Replaced truthiness checks with is not None — the standard Python idiom for distinguishing "not provided" from "provided as zero"
2. Added < 1 bounds validation — since the library is 1-based (confirmed by the start - 1 conversion and _handle_region_coords() validation), start=0 should raise a clear error rather than silently produce slice_start = -1
3. Fixed docstring typo ("Sebset" → "Subset") as noted in the issue
4. Added 3 regression tests using the existing in-memory zarr test fixture to verify correct slicing and proper error handling
5. Verified zero remaining occurrences of the buggy pattern via grep

The change is minimal and focused — only the boolean conditions and bounds checks are modified, with no structural changes.

Looking forward to your feedback!
I have opened a PR for this issue. Could you please review it?#1150

[jonbrenas]

Hi @Gopisokk, @suhr25,

Thank you for raising this to our attention. The parasite side of the API is separate from the vector side and and is not to be modified at this time.

[khushthecoder]

Hi @jonbrenas,

Understood! Since my PR (#1150) was aimed at fixing this, I am completely fine with dropping the plasmodium.py changes to leave the parasite API untouched.

However, while investigating this issue, I actually found the exact same truthiness bug on the vector side (specifically in malariagen_data/anoph/genome_sequence.py and malariagen_data/anoph/snp_data.py).

Would you like me to update my PR to remove the plasmodium changes and strictly contain the fixes for the vector side? Let me know and I can update the branch right away!

[jonbrenas]

Hi @khushthecoder, yes, removing the plasmodium changes is the way to go. Is checking that stop > 0 enough to ensure that the correct data is returned?

[khushthecoder]

Hi @jonbrenas,

I just pushed an update removing the plasmodium.py changes as requested! The PR now exclusively fixes the equivalent truthiness bugs in the anoph module (genome_sequence.py and is_accessible() in snp_data.py).

Regarding stop > 0 (resolved_region.end < 1):
Since the library expects 1-based inclusive coordinates (which is why start - 1 is used for the slice), checking that end >= 1 is a necessary bounds guard. Without it, passing end=0 generates slice_stop=0, and slicing an array like d[ :0] would quietly return an empty array.

By using standard is not None checks instead of truthiness, 0 is no longer falsely conflated with None, and the < 1 guards ensure that any out-of-bounds 0 or negative coordinates raise a clear ValueError("Region end must be >= 1 or None") instead of silently misbehaving!

Let me know if this looks good to you!