Skip to content

Accept interval columns by name regardless of order - #198

Open
buyan-kh wants to merge 1 commit into
Genentech:mainfrom
buyan-kh:fix/resize-column-order
Open

buyan-kh wants to merge 1 commit into
Genentech:mainfrom
buyan-kh:fix/resize-column-order

Conversation

@buyan-kh

@buyan-kh buyan-kh commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • check_intervals now validates chrom / start / end by column name (order-independent), so grelu.sequence.utils.resize and other callers no longer fail when those columns are present but reordered.
  • Updated resize / format docstrings to document name-based interval columns; DFSeqDataset also selects interval columns by name for consistency.
  • Added unit tests for out-of-order (and extra) interval columns in get_input_type and resize.

Fixes #34

Test plan

  • pytest tests/test_sequence.py::test_get_input_type tests/test_sequence.py::test_resize
  • Confirm tutorial-style dataframe with columns like name, end, chrom, start works with resize

check_intervals previously required chrom/start/end as the first
three columns, which broke resize and tutorials when columns were
present but reordered. Identify required columns by name, update
docstrings, select interval columns by name in DFSeqDataset, and
add unit tests for out-of-order dataframes.

Fixes Genentech#34

@avantikalal avantikalal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this — the root cause tracking is spot on. I confirmed check_intervals()'s positional column check (df.columns[:3] == ["chrom","start","end"]) and DFSeqDataset.__init__'s df.iloc[:, :3] / df.iloc[:, 3:] were the only two places in the codebase making that positional assumption (grepped for columns[:3] / iloc[:, :3] repo-wide), so this fix is complete in scope — no other callers need touching. resize() itself already indexed interval columns by name internally, so that diff is correctly docs/type-hints only.

Two things I'd like addressed before merging:

  1. Stale docstring in DFSeqDataset (src/grelu/data/dataset.py) — the class docstring still says "genomic intervals in the first 3 columns," which is no longer accurate now that column order doesn't matter. Worth updating alongside the resize()/format.py docstring changes you already made.

  2. Missing test coverage for the dataset.py change itself — the new tests (test_get_input_type, test_resize in tests/test_sequence.py) cover check_intervals/resize, but tests/test_dataset.py already has extensive DFSeqDataset coverage and gets no new case for reordered interval columns. Since DFSeqDataset.__init__'s seqs/labels split is one of the two files this PR actually changes, I'd like a test that constructs a DFSeqDataset from a dataframe with out-of-order chrom/start/end (plus label columns) and asserts the labels extracted are correct — that's the part most likely to silently regress later.

Logic itself looks correct and the existing tests are good (nice touch testing extra columns too). Requesting changes mainly for the test gap above.

This branch has not been deployed

No deployments
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.

Update doc string and input allowances for grelu.sequence.utils.resize

2 participants