Skip to content

fix(htsget): overlap behavior for enclosed regions#212

Merged
cmdoret merged 15 commits into
mainfrom
fix/htsget-enclosed-region
Jun 12, 2026
Merged

fix(htsget): overlap behavior for enclosed regions#212
cmdoret merged 15 commits into
mainfrom
fix/htsget-enclosed-region

Conversation

@cmdoret

@cmdoret cmdoret commented Jun 12, 2026

Copy link
Copy Markdown
Member

Context

modos.genomics.region.Region.overlap() does not report overlaps for completely enclosed regions.
This is illustrated by the failing test added in e0d33dc


________________________ test_overlap_contained_region _________________________

    def test_overlap_contained_region():
        """A region fully contained in another overlaps it, both ways."""
        outer = Region(chrom="chr1", start=10, end=100)
        inner = Region(chrom="chr1", start=40, end=50)
        assert outer.overlaps(inner)
>       assert inner.overlaps(outer)
E       AssertionError: assert False
E        +  where False = overlaps(Region(chrom='chr1', start=10, end=100))
E        +    where overlaps = Region(chrom='chr1', start=40, end=50).overlaps

Changes

  • overlap logic now correctly identifies contained intervals as overlaps.

@cmdoret cmdoret changed the title Fix/htsget enclosed region fix(htsget): overlap behavior for enclosed regions Jun 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes two correctness issues: genomic interval overlap detection for fully enclosed regions (used by htsget filtering), and removal of elements from multivalued link attributes in the MODO archive.

Changes:

  • Update Region.overlaps() to use proper half-open interval overlap logic (handles enclosed regions correctly).
  • Fix MODO.remove_element() to correctly remove an element from list-valued links without writing None back to Zarr attrs.
  • Add/adjust tests to cover contained-region overlap and link-list removal behavior (including preserving other links).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_region.py Adds regression test for overlaps when one region fully contains another.
tests/test_cli_local.py Updates CLI test expectation for link-list removal result ([] instead of None).
tests/test_api_local.py Adds API regression test ensuring removing one link preserves remaining links.
src/modos/genomics/region.py Fixes overlap logic using half-open interval semantics (a.start < b.end && b.start < a.end).
src/modos/api.py Fixes list-valued link removal to avoid list.remove() return-value bug.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cmdoret cmdoret self-assigned this Jun 12, 2026
@cmdoret cmdoret requested a review from almutlue June 12, 2026 12:37

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

Looks good!

@cmdoret cmdoret merged commit 4f7130f into main Jun 12, 2026
2 checks passed
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.

3 participants